-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[llvm][AArch64] Copy all operands when expanding BLR_BTI bundle #78267
Conversation
Fixes llvm#77915 Previously I based the operand copying on expandCALL_RVMARKER but did not understand it properly at the time. This lead to us dropping the arguments to the function being branched to. This fixes that by copying all operands from the BLR_BTI to the BL/BLR without skipping anything. I've updated the existing test by adding function arguments.
@llvm/pr-subscribers-backend-aarch64 Author: David Spickett (DavidSpickett) ChangesFixes #77915 Previously I based the operand copying on expandCALL_RVMARKER but did not understand it properly at the time. This lead to me dropping the arguments of the function being branched to. This fixes that by copying all operands from the BLR_BTI to the BL/BLR without skipping anything. I've updated the existing test by adding function arguments. Full diff: https://github.com/llvm/llvm-project/pull/78267.diff 3 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp b/llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
index bb7f4d907ffd7f..c45cb8eba89723 100644
--- a/llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
@@ -842,9 +842,11 @@ bool AArch64ExpandPseudo::expandCALL_BTI(MachineBasicBlock &MBB,
unsigned Opc = CallTarget.isGlobal() ? AArch64::BL : AArch64::BLR;
MachineInstr *Call =
BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(Opc)).getInstr();
- Call->addOperand(CallTarget);
+
+ for (const MachineOperand &MO : MI.operands())
+ Call->addOperand(MO);
+
Call->setCFIType(*MBB.getParent(), MI.getCFIType());
- Call->copyImplicitOps(*MBB.getParent(), MI);
MachineInstr *BTI =
BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(AArch64::HINT))
diff --git a/llvm/test/CodeGen/AArch64/blr-bti-preserves-operands.mir b/llvm/test/CodeGen/AArch64/blr-bti-preserves-operands.mir
new file mode 100644
index 00000000000000..3b2f10348bd773
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/blr-bti-preserves-operands.mir
@@ -0,0 +1,24 @@
+# RUN: llc -mtriple=aarch64-none-linux-gnu -run-pass=aarch64-expand-pseudo -o - %s | FileCheck %s
+
+# When expanding a BLR_BTI, we should copy all the operands to the branch in the
+# bundle. Otherwise we could end up using a register after the BL which was
+# clobbered by the function that was called, or overwriting an argument to that
+# function before we make the call.
+# CHECK: BUNDLE implicit-def $lr, implicit-def $w30, implicit-def $sp, implicit-def $wsp, implicit $x0, implicit $w1, implicit $sp {
+# CHECK: BL @_setjmp, $x0, $w1, csr_aarch64_aapcs, implicit-def $lr, implicit $sp, implicit-def dead $lr, implicit $sp, implicit-def $sp
+# CHECK: HINT 36
+# CHECK: }
+
+--- |
+ define void @a() {
+ ret void
+ }
+
+ declare void @_setjmp(...)
+...
+---
+name: a
+body: |
+ bb.0:
+ BLR_BTI @_setjmp, $x0, $w1, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp
+...
diff --git a/llvm/test/CodeGen/AArch64/blr-bti-preserves-regmask.mir b/llvm/test/CodeGen/AArch64/blr-bti-preserves-regmask.mir
deleted file mode 100644
index 91652c6e20c8f8..00000000000000
--- a/llvm/test/CodeGen/AArch64/blr-bti-preserves-regmask.mir
+++ /dev/null
@@ -1,23 +0,0 @@
-# RUN: llc -mtriple=aarch64-none-linux-gnu -run-pass=aarch64-expand-pseudo -o - %s | FileCheck %s
-
-# When expanding a BLR_BTI, we should keep the regmask that was attached to it.
-# Otherwise we could end up using a register after the BL which was clobbered by
-# the function that was called.
-# CHECK: BUNDLE implicit-def $lr, implicit-def $w30, implicit-def $sp, implicit-def $wsp, implicit $sp {
-# CHECK: BL @_setjmp, csr_aarch64_aapcs, implicit-def $lr, implicit $sp, implicit-def dead $lr, implicit $sp, implicit-def $sp
-# CHECK: HINT 36
-# CHECK: }
-
---- |
- define void @a() {
- ret void
- }
-
- declare void @_setjmp(...)
-...
----
-name: a
-body: |
- bb.0:
- BLR_BTI @_setjmp, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp
-...
|
@fhahn you made changes to RVMARKER recently, perhaps you can review here. What I'm doing doesn't exactly match RVMARKER, and I'm not sure of the consequences of that. Also, the comment there |
@mstorsjo I tested this using the ffmpeg commands you gave on the issue, but please run it through others if you have them. |
Thanks! If it passes |
It seems to run fine in my full build/test as well. Looking very good, thanks! |
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.
LGTM
Copying all the operands makes sense; the pseudo-instruction is basically a call instruction, so all the operands should be relevant.
Going to land this, we can look at RVMARKER later if needed. |
Thanks for fixing this issue! |
...and I forgot to try expensive checks. Revert incoming. |
…le (#78267)" This reverts commit 228aecb. Failing expensive checks: https://lab.llvm.org/buildbot/#/builders/16/builds/59798
…le (llvm#78267)" This reverts commit 955417a. The problem with the previous version was that the bundle instruction had arguments like "target arg1 arg2". When it's expanded we produced a BL or BLR which can only accept one argument, the target of the branch. Now I realise why expandCALL_RVMARKER has to copy them in mutiple steps. The operands for the called function need to be changed to implicit arguments of the branch instruction. * Copy the branch target. * Copy all register operands, marking them as implicit. * Copy any other operands without modifying them. Prior to any attempt to fix llvm#77915: BL @_setjmp, csr_aarch64_aapcs, implicit-def $lr, implicit $sp, implicit-def dead $lr, implicit $sp, implicit-def $sp Which is dropping the use of the arguments for the called function. My first fix attempt produced: BL @_setjmp, $x0, $w1, <regmask $fp ...>, implicit-def $lr, implicit $sp, implicit-def dead $lr, implicit $sp, implicit-def $sp It copied the arguments but as explicit arguments to the BL which only expects 1, failing verification. With this new change we produce: BL @_setjmp, csr_aarch64_aapcs, implicit-def $lr, implicit $sp, implicit $x0, implicit $w1, implicit-def dead $lr, implicit $sp, implicit-def $sp Note specifically the added "implicit $x0, implicit $w1". So BL only has 1 explicit argument, but the arguments to the function are still used.
…le (#78267)" (#78719) This reverts commit 955417a. The problem with the previous version was that the bundle instruction had arguments like "target arg1 arg2". When it's expanded we produced a BL or BLR which can only accept one argument, the target of the branch. Now I realise why expandCALL_RVMARKER has to copy them in mutiple steps. The operands for the called function need to be changed to implicit arguments of the branch instruction. * Copy the branch target. * Copy all register operands, marking them as implicit. * Copy any other operands without modifying them. Prior to any attempt to fix #77915: BL @_setjmp, csr_aarch64_aapcs, implicit-def $lr, implicit $sp, implicit-def dead $lr, implicit $sp, implicit-def $sp Which is dropping the use of the arguments for the called function. My first fix attempt produced: BL @_setjmp, $x0, $w1, <regmask $fp ...>, implicit-def $lr, implicit $sp, implicit-def dead $lr, implicit $sp, implicit-def $sp It copied the arguments but as explicit arguments to the BL which only expects 1, failing verification. With this new change we produce: BL @_setjmp, csr_aarch64_aapcs, implicit-def $lr, implicit $sp, implicit $x0, implicit $w1, implicit-def dead $lr, implicit $sp, implicit-def $sp Note specifically the added "implicit $x0, implicit $w1". So BL only has 1 explicit argument, but the arguments to the function are still used.
Fixes #77915
Previously I based the operand copying on expandCALL_RVMARKER but did not understand it properly at the time. This lead to me dropping the arguments of the function being branched to.
This fixes that by copying all operands from the BLR_BTI to the BL/BLR without skipping anything.
I've updated the existing test by adding function arguments.