Skip to content
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.

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

Fix unnecessary error when using neon target feature #95044

Closed

Conversation

adamgemmell
Copy link
Contributor

Fixes #95002

We previously made the decision #91608 to tie neon and fp, breaking code that just use #[target_feature(enable = "neon")]. However a deeper search revealed that this pattern was much more common than our first search suggested, including in all the aarch64/ARM neon intrinsics. Hence the proper fix should probably be to not error in this case.

r? @Amanieu

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 17, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 17, 2022
@workingjubilee
Copy link
Member

As I was saying, for all practical intents and purposes, Rust programmers do not perceive "+fp" and "+neon" to be separate entities, which is why I recommended removing "+fp" as a distinct feature entirely, because it is forward compatible to re-add it if we encounter user need for it.

@@ -325,7 +325,7 @@ pub fn from_fn_attrs<'ll, 'tcx>(

if let Some(f) = llvm_util::check_tied_features(
cx.tcx.sess,
&function_features.iter().map(|f| (*f, true)).collect(),
&mut function_features.iter().map(|f| (*f, true)).collect(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing &mut collection.iter().stuff().collect() seems like an anti-pattern to me; you're passing in a collection to be mutated, but immediately throwing it away? Its certainly not clear on the callee side that that is what you'll be doing.

I skimmed the source; it seems like all (two?) callsites are creating the hashmap and throwing it away after the call to llvm_util::check_tied_features.

If I'm correct and all call sites are indeed freshly creating the hashmap and immediately throwing it away, then I would either change check_tied_features to take the hashmap by value, or I'd revise the code for check_tied_features so that it doesn't need to modify the hashmap at all.

@Amanieu
Copy link
Member

Amanieu commented Mar 17, 2022

How exactly does this differ from the behavior prior to #90621? The original behavior was that neon implied fp, are we just going back to that behavior?

Also, why was this not caught by CI? One potential issue is that check_tied_features is being called very late in the compilation pipeline, in the codegen stage. Ideally this check should occur much sooner when the target_feature attributes are being validated. This would have helped us catch the issues in stdarch since stdarch itself would have failed to compile.

@workingjubilee
Copy link
Member

workingjubilee commented Mar 18, 2022

How exactly does this differ from the behavior prior to #90621? The original behavior was that neon implied fp, are we just going back to that behavior?

Yes, essentially. The catch is that we check the target feature restrictions on our end, before LLVM does its own feature management. So in order to be able to enforce both +neon and +fp as an additional check, we have to also add in additional feature management on our end, before we delegate to the codegen backend. By simply setting +neon, we allow any implicit subsets (as understood by the backend) to be included, but that means rustc doesn't know about the subset relationship.

This is not required in the case of the #91608 solution, which also should resolve #95002. This is why I proposed #91608: it is the solution that requires the least intervention on our part in order to make it work.

tarcieri added a commit to RustCrypto/universal-hashes that referenced this pull request Mar 21, 2022
The build is currently failing:

https://github.com/RustCrypto/universal-hashes/runs/5629586918?check_suite_focus=true

It's for two reasons:

1. `aarch64_target_feature` was recently stabilized in
   rust-lang/rust#90621 but we still include it
2. There's a bug releating to `neon`/`fp` activation. See
   rust-lang/rust#91608 and rust-lang/rust#95044

Until one of the fixes in the second issue is merged, we can't really
make progress. So this commit pins to `nightly-2022-03-01` so we can
continue to have a clean build.

Once either of the above solutions lands we can remove
`aarch64_target_feature` and unpin nightly again.
tarcieri added a commit to RustCrypto/universal-hashes that referenced this pull request Mar 21, 2022
The build is currently failing:

https://github.com/RustCrypto/universal-hashes/runs/5629586918?check_suite_focus=true

It's for two reasons:

1. `aarch64_target_feature` was recently stabilized in
   rust-lang/rust#90621 but we still include it
2. There's a bug releating to `neon`/`fp` activation. See
   rust-lang/rust#91608 and rust-lang/rust#95044

Until one of the fixes in the second issue is merged, we can't really
make progress. So this commit pins to `nightly-2022-03-01` so we can
continue to have a clean build.

Once either of the above solutions lands we can remove
`aarch64_target_feature` and unpin nightly again.
HeroicKatora added a commit to image-rs/jpeg-decoder that referenced this pull request Mar 21, 2022
The build is currently failing:

<https://github.com/image-rs/jpeg-decoder/runs/5618199478>

1. `aarch64_target_feature` was stabilized in rust-lang/rust#90621
2. `neon`/`fp` must be activated together, which is not yet the case for
   some intrinsics in `std`. See rust-lang/rust#91608 and
   rust-lang/rust#95044.

Once either of the above solutions lands we can remove
`aarch64_target_feature` and unpin nightly again.
HeroicKatora added a commit to image-rs/jpeg-decoder that referenced this pull request Mar 21, 2022
The build is currently failing:

<https://github.com/image-rs/jpeg-decoder/runs/5618199478>

1. `aarch64_target_feature` was stabilized in rust-lang/rust#90621
2. `neon`/`fp` must be activated together, which is not yet the case for
   some intrinsics in `std`. See rust-lang/rust#91608 and
   rust-lang/rust#95044.

Once either of the above solutions lands we can remove
`aarch64_target_feature` and unpin nightly again.
HeroicKatora added a commit to image-rs/jpeg-decoder that referenced this pull request Mar 24, 2022
The build is currently failing:

<https://github.com/image-rs/jpeg-decoder/runs/5618199478>

1. `aarch64_target_feature` was stabilized in rust-lang/rust#90621
2. `neon`/`fp` must be activated together, which is not yet the case for
   some intrinsics in `std`. See rust-lang/rust#91608 and
   rust-lang/rust#95044.

Once either of the above solutions lands we can remove
`aarch64_target_feature` and unpin nightly again.
HeroicKatora added a commit to image-rs/jpeg-decoder that referenced this pull request Mar 25, 2022
The build is currently failing:

<https://github.com/image-rs/jpeg-decoder/runs/5618199478>

1. `aarch64_target_feature` was stabilized in rust-lang/rust#90621
2. `neon`/`fp` must be activated together, which is not yet the case for
   some intrinsics in `std`. See rust-lang/rust#91608 and
   rust-lang/rust#95044.

Once either of the above solutions lands we can remove
`aarch64_target_feature` and unpin nightly again.
wartmanm pushed a commit to wartmanm/jpeg-decoder that referenced this pull request Sep 15, 2022
The build is currently failing:

<https://github.com/image-rs/jpeg-decoder/runs/5618199478>

1. `aarch64_target_feature` was stabilized in rust-lang/rust#90621
2. `neon`/`fp` must be activated together, which is not yet the case for
   some intrinsics in `std`. See rust-lang/rust#91608 and
   rust-lang/rust#95044.

Once either of the above solutions lands we can remove
`aarch64_target_feature` and unpin nightly again.
drunest added a commit to drunest/jpegdecoder that referenced this pull request Jun 28, 2024
The build is currently failing:

<https://github.com/image-rs/jpeg-decoder/runs/5618199478>

1. `aarch64_target_feature` was stabilized in rust-lang/rust#90621
2. `neon`/`fp` must be activated together, which is not yet the case for
   some intrinsics in `std`. See rust-lang/rust#91608 and
   rust-lang/rust#95044.

Once either of the above solutions lands we can remove
`aarch64_target_feature` and unpin nightly again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aarch64 neon intrinsics broken after #90621
6 participants