Fix some issues with folded AArch64 features #107294

JamieCunliffe · 2023-01-25T13:15:12Z

In #91608 the fp feature was removed for AArch64 and folded into the neon feature, however disabling the neon feature doesn't actually disable the fp feature. If my understanding on that thread is correct it should do.

While doing this, I also noticed that disabling some features would disable features that it shouldn't. For instance enabling sve will enable neon, however, when disabling sve it would then also disable neon, I wouldn't expect disabling sve to also disable neon.

cc @workingjubilee

rustbot · 2023-01-25T13:15:22Z

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

oli-obk · 2023-01-25T13:29:38Z

r? @Amanieu since you reviewed the other PR, too

Amanieu · 2023-01-31T21:13:12Z

compiler/rustc_codegen_llvm/src/llvm_util.rs

@@ -149,6 +149,9 @@ pub fn time_trace_profiler_finish(file_name: &Path) {
 // Though note that Rust can also be build with an external precompiled version of LLVM
 // which might lead to failures if the oldest tested / supported LLVM version
 // doesn't yet support the relevant intrinsics
+//
+// Note: The first feature in the list that is returned is the mapping to the feature that is
+// provided from the `s` parameter.
 pub fn to_llvm_features<'a>(sess: &Session, s: &'a str) -> SmallVec<[&'a str; 2]> {


I think it's a mistake to make this function do too much. Could the logic for implied/tied features be moved to a separate function and handled separately?

Yeah, I agree with that and was thinking the same while making the change. This was already handling the implied features before this change, so I just added neon and fp-armv8 to it to fix that issue. The fact it's used in multiple places is why I created a different function for if they should be disabled together.

It was done this way to fix those bugs, however, long term I think this should be a graph structure to correctly show the relationships between the different features.

Perhaps it would be clearer to make this function return a struct instead:

struct LlvmFeatures<'a> { /// Features that should be enabled/disabled. features: SmallVec<[&'a str; 2]>, /// Features that are implicitly enabled when the feature is enabled. dependencies: SmallVec<[&'a str; 2]>, }

apiraino · 2023-03-01T14:06:12Z

If I read correctly the last comment, this can be switched to waiting on author to incorporate changes. Feel free to request a review with @rustbot ready, thanks!

@rustbot author

Dylan-DPC · 2023-05-18T17:56:08Z

@JamieCunliffe any updates on this?

In rust-lang#91608 the fp-armv8 feature was removed as it's tied to the neon feature. However disabling neon didn't actually disable the use of floating point registers and instructions, for this `-fp-armv8` is required.

Some features that are tied together only make sense to be folded together when enabling the feature. For example on AArch64 sve and neon are tied together, however it doesn't make sense to disable neon when disabling sve.

Rather than returning an array of features from to_llvm_features, return a structure that contains the dependencies. This also contains metadata on how the features depend on each other to allow for the correct enabling and disabling.

Amanieu · 2023-05-23T12:12:38Z

@bors r+

bors · 2023-05-23T12:12:40Z

📌 Commit 04efd0da1cfb438c5cb41b6d3637e719416faf0f has been approved by Amanieu

It is now in the queue for this repository.

JamieCunliffe · 2023-05-23T12:52:47Z

@Amanieu I'm not sure if it needs to be approved again, but I updated it to resolve the merge conflicts.

Amanieu · 2023-05-23T13:12:26Z

@bors r+

bors · 2023-05-23T13:12:27Z

📌 Commit a059e68 has been approved by Amanieu

It is now in the queue for this repository.

bors · 2023-05-23T15:30:40Z

⌛ Testing commit a059e68 with merge 52dd1cd...

bors · 2023-05-23T18:13:30Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 52dd1cd to master...

rust-timer · 2023-05-23T19:40:04Z

Finished benchmarking commit (52dd1cd): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-4.8%	[-4.8%, -4.8%]	1
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	1
All ❌✅ (primary)	-4.8%	[-4.8%, -4.8%]	1

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)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 646.753s -> 646.927s (0.03%)

rustbot assigned oli-obk Jan 25, 2023

rustbot added 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. labels Jan 25, 2023

rustbot assigned Amanieu and unassigned oli-obk Jan 25, 2023

This comment has been minimized.

Sign in to view

ojeda mentioned this pull request Jan 25, 2023

rustc wanted features & bugfixes Rust-for-Linux/linux#355

Open

48 tasks

Amanieu reviewed Jan 31, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2023

JamieCunliffe force-pushed the neon-fp branch from 1b45d4c to 04efd0d Compare May 22, 2023 13:23

JamieCunliffe added 4 commits May 22, 2023 14:27

Tie neon with fp-armv8.

4cca436

In rust-lang#91608 the fp-armv8 feature was removed as it's tied to the neon feature. However disabling neon didn't actually disable the use of floating point registers and instructions, for this `-fp-armv8` is required.

Only disable folded features when it makes sense.

aab0757

Some features that are tied together only make sense to be folded together when enabling the feature. For example on AArch64 sve and neon are tied together, however it doesn't make sense to disable neon when disabling sve.

Make v8a match optional in the test feature list.

d51db42

Dylan-DPC added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 22, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2023

JamieCunliffe force-pushed the neon-fp branch from 04efd0d to a059e68 Compare May 23, 2023 12:49

bors added the merged-by-bors This PR was explicitly merged by bors. label May 23, 2023

bors merged commit 52dd1cd into rust-lang:master May 23, 2023

rustbot added this to the 1.71.0 milestone May 23, 2023

bors mentioned this pull request May 23, 2023

use c literals in compiler and library #111647

Merged

Skepfyr mentioned this pull request Jan 6, 2024

regression: overflow evaluating requirement #113239

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some issues with folded AArch64 features #107294

Fix some issues with folded AArch64 features #107294

JamieCunliffe commented Jan 25, 2023

rustbot commented Jan 25, 2023

oli-obk commented Jan 25, 2023

This comment has been minimized.

Amanieu Jan 31, 2023

JamieCunliffe Feb 2, 2023

Amanieu Feb 3, 2023 •

edited

Loading

apiraino commented Mar 1, 2023

Dylan-DPC commented May 18, 2023

Amanieu commented May 23, 2023

bors commented May 23, 2023

JamieCunliffe commented May 23, 2023

Amanieu commented May 23, 2023

bors commented May 23, 2023

bors commented May 23, 2023

bors commented May 23, 2023

rust-timer commented May 23, 2023

Fix some issues with folded AArch64 features #107294

Fix some issues with folded AArch64 features #107294

Conversation

JamieCunliffe commented Jan 25, 2023

rustbot commented Jan 25, 2023

oli-obk commented Jan 25, 2023

This comment has been minimized.

Amanieu Jan 31, 2023

Choose a reason for hiding this comment

JamieCunliffe Feb 2, 2023

Choose a reason for hiding this comment

Amanieu Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

apiraino commented Mar 1, 2023

Dylan-DPC commented May 18, 2023

Amanieu commented May 23, 2023

bors commented May 23, 2023

JamieCunliffe commented May 23, 2023

Amanieu commented May 23, 2023

bors commented May 23, 2023

bors commented May 23, 2023

bors commented May 23, 2023

rust-timer commented May 23, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Amanieu Feb 3, 2023 •

edited

Loading