[AArch64] support .arch_extension for features that the CLI also accepts
#169999
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-backend-aarch64 Author: Folkert de Vries (folkertdev) Changesfixes #146866 The issue discusses that it is unfortunate that command line flag parsing and assembly parsing don't share the infrastructure for recognizing features, which can lead to inconsistencies like here. I don't really see how I can combine them though, so for now this change will fix the immediate problem. Full diff: https://github.com/llvm/llvm-project/pull/169999.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp b/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
index 433cb0387c470..7116fd0ea3b6f 100644
--- a/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
+++ b/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
@@ -3954,6 +3954,7 @@ static const struct Extension {
{"poe2", {AArch64::FeatureS1POE2}},
{"tev", {AArch64::FeatureTEV}},
{"btie", {AArch64::FeatureBTIE}},
+ {"dit", {AArch64::FeatureDIT}},
};
static void setRequiredFeatureString(FeatureBitset FBS, std::string &Str) {
diff --git a/llvm/test/MC/AArch64/directive-arch_extension.s b/llvm/test/MC/AArch64/directive-arch_extension.s
index 3c754077572a1..f174e9d4d187e 100644
--- a/llvm/test/MC/AArch64/directive-arch_extension.s
+++ b/llvm/test/MC/AArch64/directive-arch_extension.s
@@ -197,3 +197,7 @@ fmmla v1.8h, v2.16b, v3.16b
.arch_extension f8f32mm
fmmla v1.4s, v2.16b, v3.16b
// CHECK: fmmla v1.4s, v2.16b, v3.16b
+
+.arch_extension dit
+msr DIT, #1
+// CHECK: msr DIT, #1
|
davemgreen
requested review from
davemgreen,
efriedma-quic,
jthackray and
ostannard
Collaborator
|
GCC doesn't support this (https://gcc.godbolt.org/z/EcvKh4Tjv), but if there is a -march option then it sounds OK to add one. The other alternative is to remove the need for +dit, like GCC has it. See for example #163166. I believe there is the idea that sys-reg only extensions do not give an error, as you can only write the assembly where you know they are useful. I think either is fine, I'm not sure what others think and whether I've mis-remembered the idea of MSR sysregs. |
Collaborator
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
I think the set of extensions we support with march should be the same as the list here.
Going through the list, the following are missing:
brbe
bti
fcma
jscvt
pauth-lr
pmuv3
ssve-fexpa
wfxt
Maybe we can do them all in one batch?
I think this is worth fixing regardless of whether we require dit for the asm parser to parse msr DIT.
Contributor
Author
|
I added all of those except for In general the names are not always consistent. I'm using the name that shows up in the error message when the feature is not present. |
Collaborator
|
We shouldn't be exposing "complxnum" to users; it's the internal name which... I guess we picked before there was a standard name. If it's showing up in error messages, that's a bug in the diagnostic code. |
folkertdev
force-pushed
the
aarch64-asm-dit
branch
from
63d960c to
cc4c030
Compare
Contributor
Author
|
I fixed that one so What about |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
Contributor
Author
|
Looks like the https://github.com/llvm/llvm-project/blob/main/llvm/test/MC/AArch64/armv8.3a-complex_missing.s So, can that just be changed? |
Collaborator
-mattr is the LLVM internal notion of "attributes", not the architecture extension names defined by Arm. They mostly match, but not completely, for historical reasons.
It's fine if the diagnostics don't match the -mattr flag. |
folkertdev
force-pushed
the
aarch64-asm-dit
branch
from
cc4c030 to
dc0a1fd
Compare
efriedma-quic
approved these changes
Dec 2, 2025
Collaborator
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
Please make sure the commit message reflects the extended change. Otherwise LGTM, but maybe give a couple days if anyone has a second opinion.
Contributor
Author
|
I don't have merge permissions, but it's been a couple of days now, so could someone merge this? |
Collaborator
I'm reading this change for the first time and I don't believe that this has been done, or you updated the literal commit message but not the PR description. The way LLVM merges PRs is the commit message of the final merged commit is taken from the PR description. Which is not that obvious but it is what it is. So please update the PR description (#169999 (comment)) and PR title and probably we can merge this today given the approvals that it has. |
folkertdev
changed the title
.arch_extension dit.arch_extension on features that the CLI can parse
folkertdev
changed the title
.arch_extension on features that the CLI can parse.arch_extension on features that the CLI accepts
Contributor
Author
|
Right, I updated the text |
DavidSpickett
changed the title
.arch_extension on features that the CLI accepts.arch_extension for features that the CLI also accepts
Collaborator
|
Thanks! I've updated the branch and enabled the auto-merge so if CI is green (chances are it will be), it will be merged. |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/94/builds/13442 Here is the relevant piece of the build log for the reference |
Collaborator
|
You can ignore the above failure, it was green next build. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #146866
The CLI and
.arch_extensionuse a different list of features, and some features that the CLI supports cannot currently be toggled using.arch_extension. This PR fixes that, adding support for.arch_extensionfor the following features:ditbrbebtifcmajscvtpauthssvewfxtThe issue discusses that it is unfortunate that command line flag parsing and assembly parsing don't share the infrastructure for recognizing features, which can lead to inconsistencies like here. I don't really see how I can combine them though, so for now this change will fix the immediate problem.