-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[AArch64] support .arch_extension dit
#169999
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
base: main
Are you sure you want to change the base?
Conversation
|
@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
|
|
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. |
efriedma-quic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
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. |
|
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. |
63d960c to
cc4c030
Compare
|
I fixed that one so What about |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
|
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? |
-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. |
cc4c030 to
dc0a1fd
Compare
efriedma-quic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure the commit message reflects the extended change. Otherwise LGTM, but maybe give a couple days if anyone has a second opinion.
fixes #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.