[AArch64] Fix handling of x29/x30 in inline assembly clobbers #167783
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-backend-aarch64 Author: Lukáš Lalinský (lalinsky) ChangesThe AArch64 backend was silently ignoring inline assembly clobbers when numeric register names ( There is an incomlete workaround for this in Rust, but that only handles This patch adds explicit handling in Full diff: https://github.com/llvm/llvm-project/pull/167783.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index eaa10ef031989..b3e08f3522fcc 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -30,6 +30,7 @@
#include "llvm/ADT/SmallVectorExtras.h"
#include "llvm/ADT/Statistic.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Analysis/LoopInfo.h"
#include "llvm/Analysis/MemoryLocation.h"
@@ -13126,6 +13127,19 @@ AArch64TargetLowering::getRegForInlineAsmConstraint(
return std::make_pair(unsigned(AArch64::ZT0), &AArch64::ZTRRegClass);
}
+ // Clang will correctly decode the usage of register name aliases into their
+ // official names. However, other frontends like `rustc` do not. This allows
+ // users of these frontends to use the ABI names for registers in LLVM-style
+ // register constraints.
+ unsigned XRegFromAlias = StringSwitch<unsigned>(Constraint.lower())
+ .Cases({"{x29}", "{fp}"}, AArch64::FP)
+ .Cases({"{x30}", "{lr}"}, AArch64::LR)
+ .Cases({"{x31}", "{sp}"}, AArch64::SP)
+ .Default(AArch64::NoRegister);
+ if (XRegFromAlias != AArch64::NoRegister) {
+ return std::make_pair(XRegFromAlias, &AArch64::GPR64RegClass);
+ }
+
// Use the default implementation in TargetLowering to convert the register
// constraint into a member of a register class.
std::pair<unsigned, const TargetRegisterClass *> Res;
diff --git a/llvm/test/CodeGen/AArch64/inline-asm-clobber-x29-x30.ll b/llvm/test/CodeGen/AArch64/inline-asm-clobber-x29-x30.ll
new file mode 100644
index 0000000000000..587a85367b5ba
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/inline-asm-clobber-x29-x30.ll
@@ -0,0 +1,36 @@
+; RUN: llc -mtriple=aarch64 -verify-machineinstrs < %s | FileCheck %s
+
+; Test that both numeric register names (x29, x30) and their architectural
+; aliases (fp, lr) work correctly as clobbers in inline assembly.
+
+define void @clobber_x29() nounwind {
+; CHECK-LABEL: clobber_x29:
+; CHECK: str x29, [sp
+; CHECK: ldr x29, [sp
+ tail call void asm sideeffect "", "~{x29}"()
+ ret void
+}
+
+define void @clobber_fp() nounwind {
+; CHECK-LABEL: clobber_fp:
+; CHECK: str x29, [sp
+; CHECK: ldr x29, [sp
+ tail call void asm sideeffect "", "~{fp}"()
+ ret void
+}
+
+define void @clobber_x30() nounwind {
+; CHECK-LABEL: clobber_x30:
+; CHECK: str x30, [sp
+; CHECK: ldr x30, [sp
+ tail call void asm sideeffect "", "~{x30}"()
+ ret void
+}
+
+define void @clobber_lr() nounwind {
+; CHECK-LABEL: clobber_lr:
+; CHECK: str x30, [sp
+; CHECK: ldr x30, [sp
+ tail call void asm sideeffect "", "~{lr}"()
+ ret void
+}
|
Definitely, thanks for contributing! Do you know where the Rust workaround is and if there's any tracking issue for it? so we have some way to notify them that this exists. |
|
|
The CI failures are interesting, they are actually testing the incorrect behavior: |
|
Could you open an issue in Rust for this? I don't know how they handle backports/updates/cherry-picks but I'm sure someone over there can do the right thing for it if you open an issue. |
lalinsky
force-pushed
the
aarch64-reg-aliases
branch
from
e2036ea to
3eda571
Compare
|
Opened issue in Rust to remove the workaround later: |
Error: Command failed due to missing milestone. |
Thanks! (ignore the bot comment, it's look for |
This comment was marked as resolved.
This comment was marked as resolved.
lalinsky
changed the title
lalinsky
force-pushed
the
aarch64-reg-aliases
branch
2 times, most recently
from
2ed12ee to
128ac4f
Compare
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
I was able to confirm the missing save/restore with https://godbolt.org/z/ETjrPc1nG
Please add an example to the commit message showing the before/after.
I understand the change and the new test cases, but am not sure why each of the existing tests had to change. Can you explain?
I expected that the current use should still work, or if it didn't, all existing tests would remove x30 and x31, but some of them have x29 and x31 removed.
I'm missing something but regardless, please point it out for me :)
|
Also update the PR description now that this is only x29/x30. |
fp/sp can't be really marked as clobbered, they trigger warnings, and those warnings failed the tests. they literally depended on the fact that x29/x30/x31 are completely ignored. there is even the one test I mentioned in an earlier comment, that tries to clobber everything but x30, likely to show that llvm can use x30 for its own purposes in such situatoins, but that previously worked just by the fact that all of them were ignored. after I changed the behavior of x29, so that it clobbers fp, the behavior of the tests changed. I can try again, now that I removed x31 from the mapping, but it seems silly to have x31 in the tests and depend on it being ignored. |
Thanks, I didn't manage to connect those two things. Your added code means we actually try to handle them and realise we need to warn the user, before we didn't even check.
It is silly, but I think that's part of a further problem there that we should warn for x31 too (you don't have to fix this yourself though). I'd prefer this PR to be only changes related to x29/x30. |
|
Ok, I'll restrict this to just x29/x30 and update everything accordingly The one test is sketchy though. I don't fully understand it, but I think the CHECKS actually check that the compiler can store data in x29, despite it being added to the clobber list. A proper fix would be update the checks to make sure it uses x30, which is not clobbered. I'd like to do that, but I'd have to dig a lot deeper to understand the exact output. |
lalinsky
force-pushed
the
aarch64-reg-aliases
branch
from
128ac4f to
448a86d
Compare
The AArch64 backend was silently ignoring inline assembly clobbers when
numeric register names (x29, x30) were used instead of their
architectural aliases (fp, lr). I found this bug via inline assembly
in Zig, which not normalize the register names the way clang does.
There is an incoplete workaround for this in Rust, but that only
handles `x30/lr`, not `x29/fp`. I thought it would make
sense to fix this properly rather than adding a workaround to Zig.
This patch adds explicit handling in getRegForInlineAsmConstraint() to
map both numeric and alias forms to the correct physical registers,
following the same pattern used by the RISC-V backend.
I've left `x31/sp` without changes, it would nice to have to have
warning when trying to clobber `x31`, just like there is for `sp`,
but that register needs different handling, so it's best done
separately.
If you have code like this:
define void @clobber_x30() nounwind {
tail call void asm sideeffect "nop", "~{x30}"()
ret void
}
Here is the generated assembly before:
clobber_x30: // @clobber_x30
//APP
nop
//NO_APP
ret
And after:
clobber_x30: // @clobber_x30
str x30, [sp, #-16]! // 8-byte Folded Spill
//APP
nop
//NO_APP
ldr x30, [sp], llvm#16 // 8-byte Folded Reload
ret
lalinsky
force-pushed
the
aarch64-reg-aliases
branch
from
9d22f7f to
82eb730
Compare
|
I've added the x31/sp comment, updated commit message to include assembly before/after, reverted the test change as they only had problem with the x31/sp mapping, not x29/fp, updated PR description. |
There was a problem hiding this comment.
I think this needs a release note in https://github.com/llvm/llvm-project/blob/main/llvm/docs/ReleaseNotes.md#changes-to-the-aarch64-backend.
I suggest:
- A bug was fixed that caused LLVM IR inline assembly clobbers of the
x29andx30registers to be ignored when they were written using theirxNnames instead of the ABI namesFPandLR. Note that LLVM IR produced by Clang always uses the ABI names, but other frontends may not. (#link-to-this-pr)
Feel free to copy paste that if you think it's correct. And more importantly, if you had never found this problem, and read this release note during a Zig update, what would you want to see there?
| ; CHECK-LABEL: clobber_x29: | ||
| ; CHECK: str x29, [sp | ||
| ; CHECK: ldr x29, [sp | ||
| tail call void asm sideeffect "", "~{x29}"() |
There was a problem hiding this comment.
I know these are marked tailcall so in theory we should never get load/stores for stack frame management.
However, if you put a nop in the asm statement, you could make this into:
CHECK: str
CHECK-NEXT: nop
CHECK-NEXT: ldr
And it would be bit more robust and immediately clear what the intended behaviour is.
There was a problem hiding this comment.
Well, you stated the test intent above (thank you for that too, it often gets forgotten), but it can be visually more clear.
3 participants
The AArch64 backend was silently ignoring inline assembly clobbers when
numeric register names (x29, x30) were used instead of their
architectural aliases (fp, lr). I found this bug via inline assembly
in Zig, which not normalize the register names the way clang does.
There is an incoplete workaround for this in Rust, but that only
handles
x30/lr, notx29/fp. I thought it would makesense to fix this properly rather than adding a workaround to Zig.
This patch adds explicit handling in getRegForInlineAsmConstraint() to
map both numeric and alias forms to the correct physical registers,
following the same pattern used by the RISC-V backend.
I've left
x31/spwithout changes, it would nice to have to havewarning when trying to clobber
x31, just like there is forsp,but that register needs different handling, so it's best done
separately.
If you have code like this:
Here is the generated assembly before:
And after: