New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add loongarch64 asm! support #101069

Merged

bors merged 1 commit into rust-lang:master from zhaixiaojuan:loongarch64-inline-asm

Apr 25, 2023

Contributor

zhaixiaojuan commented Aug 27, 2022

No description provided.

rust-highfive assigned cjgillot

Collaborator

rust-highfive commented Aug 27, 2022

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon.

Please see the contribution instructions for more information.

rustbot added the T-compiler label

rust-highfive added the S-waiting-on-review label

This comment has been minimized.

Sign in to view

zhaixiaojuan force-pushed the loongarch64-inline-asm branch from edefffb to f3fcaf1 Compare

August 27, 2022 05:11

Collaborator

rustbot commented Aug 27, 2022

Some changes occurred in compiler/rustc_codegen_gcc

This comment has been minimized.

Sign in to view

Contributor

cjgillot commented Aug 27, 2022

As it's related to asm, @Amanieu could you take a look?

Member

Amanieu commented Aug 27, 2022

LGTM but this needs tests in src/test/assembly/asm.

zhaixiaojuan force-pushed the loongarch64-inline-asm branch from f3fcaf1 to 8dbc47d Compare

September 5, 2022 12:00

This comment has been minimized.

Sign in to view

Contributor Author

zhaixiaojuan commented Sep 5, 2022

src/test/assembly/asm/loongarch-type.rs generates the corresponding assembly file: loongarch-type.s

Member

Amanieu commented Sep 5, 2022

It seems that CI is failing due to a check introduced by #77280.

cc @petrochenkov: The loongarch target was added in LLVM 15, and is not available on the PR CI which uses the system LLVM.

Member

Amanieu commented Sep 5, 2022

I think you might be able to fix this by adding // min-llvm-version: 15.0 to that file.

zhaixiaojuan force-pushed the loongarch64-inline-asm branch from 8dbc47d to 419eb1a Compare

September 5, 2022 13:33

Contributor Author

zhaixiaojuan commented Sep 5, 2022

I think you might be able to fix this by adding // min-llvm-version: 15.0 to that file.

Thanks a lot.

This comment has been minimized.

Sign in to view

Member

bjorn3 commented Sep 5, 2022

When COMPILETEST_NEEDS_ALL_LLVM_COMPONENTS is set, compiletest expects that all llvm components used by any test are available. This env var is set in CI. In other words you can't use make a test conditional on any llvm component that isn't enabled by default for rust it seems. If loongarch64 is already supported by the llvm version used by rust, you could add it to

rust/src/bootstrap/native.rs

Line 358 in b32223f

None => "AVR;M68k",

to enable it. (also make sure to add it to the experimental_targets field in config.toml.example).

Member

Amanieu commented Sep 5, 2022

I think you need to adjust this check:

rust/src/tools/compiletest/src/header.rs

Line 1037 in b32223f

if let Some(needed_components) =

Move the component detection after the LLVM version checking.

zhaixiaojuan force-pushed the loongarch64-inline-asm branch from 419eb1a to 530dedc Compare

September 15, 2022 06:38

This comment has been minimized.

Sign in to view

zhaixiaojuan force-pushed the loongarch64-inline-asm branch from 530dedc to f98a9fd Compare

September 16, 2022 03:53

Contributor Author

zhaixiaojuan commented Sep 16, 2022

I adjusted the check order of llvm version and component in the file
src/tools/compiletest/src/header.rs.

Amanieu reviewed

View reviewed changes

src/test/assembly/asm/loongarch-type.rs Outdated

    
            @@ -0,0 +1,195 @@
          
              // min-llvm-version: 15.0

              // revisions: loongarch64

Member

Amanieu Sep 16, 2022

This line is not needed.

src/tools/compiletest/src/header.rs Outdated

    
                  if let Some(actual_version) = config.llvm_version {

                      if let Some(rest) = line.strip_prefix("min-llvm-version:").map(str::trim) {

                          let min_version = extract_llvm_version(rest).unwrap();

                          // Ignore if actual version is smaller the minimum required

                          // version

                          actual_version < min_version

                          return actual_version < min_version;

Member

Amanieu Sep 16, 2022

Suggested change

      
                        return actual_version < min_version;
          
                        if actual_version < min_version {
          
                            return true;
          
                        }

src/tools/compiletest/src/header.rs Outdated

    
                      } else if let Some(rest) = line.strip_prefix("min-system-llvm-version:").map(str::trim) {

                          let min_version = extract_llvm_version(rest).unwrap();

                          // Ignore if using system LLVM and actual version

                          // is smaller the minimum required version

                          config.system_llvm && actual_version < min_version

                          return config.system_llvm && actual_version < min_version;

Member

Amanieu Sep 16, 2022

Same here.

src/tools/compiletest/src/header.rs Outdated

    
            @@ -1069,11 +1055,27 @@ fn ignore_llvm(config: &Config, line: &str) -> bool {
          
                              panic!("Malformed LLVM version range: max < min")

                          }

                          // Ignore if version lies inside of range.

                          actual_version >= v_min && actual_version <= v_max

                          return actual_version >= v_min && actual_version <= v_max;

Member

Amanieu Sep 16, 2022

Same here.

src/tools/compiletest/src/header.rs Outdated

    
                      } else {

                          false

                          return false;

Member

Amanieu Sep 16, 2022

Remove the else branch: if there's no min-llvm-version then we should continue on to the next check below.

src/tools/compiletest/src/header.rs Outdated

Comment on lines 1074 to 1076

    
                          return true;

                      } else {

                          return false;

Member

Amanieu Sep 16, 2022

Suggested change

      
                        return true;
          
                    } else {
          
                        return false;
          
                        true
          
                    } else {
          
                        false

Use trailing return syntax.

src/tools/compiletest/src/header.rs Outdated

    
                      }

                  } else {

                      false

                      return false;

Member

Amanieu Sep 16, 2022

Suggested change

      
                    return false;
          
                    false

zhaixiaojuan force-pushed the loongarch64-inline-asm branch from f98a9fd to ce51141 Compare

September 17, 2022 08:58

Contributor Author

zhaixiaojuan commented Sep 17, 2022

I seem to find a problem, when the iter_header function is called in make_test_description, the iter_header function loops to read the configuration information of each line in the xxx-types.rs file, and ignore_llvm is called every time during the loop.
When reading min-llvm-revision, the min-llvm-version branch will be entered in the llvm-ignore function. But when reading the component, the version check will be skipped and go directly to the component branch, which results in a "missing LLVM component: loongarch" error.

make_test_description:

iter_header:

I'm not sure if my thinking is correct, my current understanding is:
Between lines 924 and 948 of the code, I think the logic here should be that as long as there is a configuration that makes the value of ignore true, then the value of ignore will be true at the end.
Therefore, when ignore is set to true in one loop of iter_header, the value of ignore does not need to be set in subsequent loops.So when setting the ignore value, I added the judgment condition.

src/tools/compiletest/src/header.rs
make_test_description:

923        if !ignore {
924            ignore |= ignore_llvm(config, ln);
 ......
948            ignore |= !has_rust_lld && config.parse_name_directive(ln, "needs-rust-lld");
949        }

Member

Amanieu commented Sep 18, 2022

Contributor

bors commented Sep 18, 2022

📌 Commit ce51141ae3f5fa8f0ed8d0bdac8956e91e90ddb3 has been approved by Amanieu

It is now in the queue for this repository.

bors removed the S-waiting-on-review label

Contributor

heiher commented Apr 12, 2023

@Amanieu I recently updated the patch set and noticed that one of the CI tests failed. It appears that the failling test is bound to an older version that does not support LoongArch. I would greatly appreciate it if you could provide me with some guidance on how to resolve this issue.

Contributor

heiher commented Apr 12, 2023

@Amanieu I recently updated the patch set and noticed that one of the CI tests failed. It appears that the failling test is bound to an older version that does not support LoongArch. I would greatly appreciate it if you could provide me with some guidance on how to resolve this issue.

It seems we should check the version before checking the components to ignore tests that do not match the min-llvm-version.

Amanieu added a commit to Amanieu/rust that referenced this pull request


          Allow older LLVM versions to have missing components

7e8611b

This check was introduced by rust-lang#77280 to ensure that all tests that are
filtered by LLVM component are actually tested in CI. However this
causes issues for new targets (e.g. rust-lang#101069) where support is only
available on the latest LLVM version.

This PR restricts the tests to only CI jobs that use the latest LLVM
version.

Amanieu mentioned this pull request

Allow older LLVM versions to have missing components #110232

Merged

Member

Amanieu commented Apr 12, 2023

Should be fixed by #110232.

Amanieu reviewed

View reviewed changes

Member

Amanieu left a comment

Can you also update the documentation in the unstable book for src/doc/unstable-book/src/language-features/asm-experimental-arch.md.

compiler/rustc_ast_lowering/src/asm.rs Outdated

    
            @@ -44,6 +44,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
          
                                  | asm::InlineAsmArch::AArch64

                                  | asm::InlineAsmArch::RiscV32

                                  | asm::InlineAsmArch::RiscV64

                                  | asm::InlineAsmArch::LoongArch64

Member

Amanieu Apr 12, 2023

Loongarch asm is not stable, it should not be included in this list.

zhaixiaojuan force-pushed the loongarch64-inline-asm branch 2 times, most recently from f57183f to 8b61821 Compare

April 13, 2023 01:07

rustbot added A-testsuite T-infra labels

Contributor

bors commented Apr 13, 2023

☔ The latest upstream changes (presumably #109989) made this pull request unmergeable. Please resolve the merge conflicts.

zhaixiaojuan force-pushed the loongarch64-inline-asm branch from 8b61821 to 2abcc61 Compare

April 13, 2023 15:28

xen0n mentioned this pull request

Add support for loongarch64 bytecodealliance/rustix#528

Closed

bors added a commit that referenced this pull request


          Auto merge of #110232 - Amanieu:old-llvm-components, r=petrochenkov

f6830a2

Allow older LLVM versions to have missing components

This check was introduced by #77280 to ensure that all tests that are filtered by LLVM component are actually tested in CI. However this causes issues for new targets (e.g. #101069) where support is only available on the latest LLVM version.

This PR restricts the tests to only CI jobs that use the latest LLVM version.


          Add loongarch64 asm! support

5f2fa4c

zhaixiaojuan force-pushed the loongarch64-inline-asm branch from 2abcc61 to 5f2fa4c Compare

April 25, 2023 06:15

Member

Amanieu commented Apr 25, 2023

Contributor

bors commented Apr 25, 2023

📌 Commit 5f2fa4c has been approved by Amanieu

It is now in the queue for this repository.

bors added S-waiting-on-bors and removed S-blocked labels

Contributor

bors commented Apr 25, 2023

⌛ Testing commit 5f2fa4c with merge 999e6e5...

Contributor

bors commented Apr 25, 2023

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 999e6e5 to master...

bors added the merged-by-bors label

bors merged commit 999e6e5 into rust-lang:master

rustbot added this to the 1.71.0 milestone

Collaborator

rust-timer commented Apr 25, 2023

Finished benchmarking commit (999e6e5): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.2%	[1.1%, 2.9%]	3
Regressions ❌ (secondary)	4.9%	[0.3%, 8.2%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.2%	[1.1%, 2.9%]	3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.0%, -3.0%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	[-1.3%, -1.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.3%	[-1.3%, -1.3%]	1

rustbot added the perf-regression label

Member

lqd commented Apr 25, 2023

cranelift-codegen / keccak are currently noisy, and this PR looks completely unrelated, @rustbot label: +perf-regression-triaged.

rustbot added the perf-regression-triaged label

antoyo pushed a commit to antoyo/rust that referenced this pull request


          Auto merge of rust-lang#101069 - zhaixiaojuan:loongarch64-inline-asm,…

403ebdc

… r=Amanieu

Add loongarch64 asm! support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-testsuite merged-by-bors perf-regression perf-regression-triaged S-waiting-on-bors T-compiler T-infra