[AArch64] Fix stp kill when merging forward. #152994
Merged
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
|
@llvm/pr-subscribers-backend-aarch64 Author: Ricardo Jesus (rj-jesus) ChangesAs an alternative to #149177, iterate through all instructions in Full diff: https://github.com/llvm/llvm-project/pull/152994.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
index 782d62a7e5e13..e69fa32967a79 100644
--- a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
+++ b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
@@ -1193,7 +1193,8 @@ AArch64LoadStoreOpt::mergePairedInsns(MachineBasicBlock::iterator I,
// USE kill %w1 ; need to clear kill flag when moving STRWui downwards
// STRW %w0
Register Reg = getLdStRegOp(*I).getReg();
- for (MachineInstr &MI : make_range(std::next(I), Paired))
+ for (MachineInstr &MI :
+ make_range(std::next(I->getIterator()), Paired->getIterator()))
MI.clearRegisterKills(Reg, TRI);
}
}
diff --git a/llvm/test/CodeGen/AArch64/sve-vls-ldst-opt.mir b/llvm/test/CodeGen/AArch64/sve-vls-ldst-opt.mir
index 49453bc178914..a3e6cc10c1d0b 100644
--- a/llvm/test/CodeGen/AArch64/sve-vls-ldst-opt.mir
+++ b/llvm/test/CodeGen/AArch64/sve-vls-ldst-opt.mir
@@ -72,3 +72,21 @@ body: |
# CHECK: STURQi killed renamable $q1, renamable $x1, 16 :: (store (s128))
# CHECK: STURQi killed renamable $q2, renamable $x1, 48 :: (store (s128))
# CHECK: STR_ZXI killed renamable $z3, renamable $x1, 4 :: (store (<vscale x 1 x s128>))
+---
+name: clear-kill-in-bundle-forward
+tracksRegLiveness: true
+body: |
+ bb.0:
+ liveins: $x0, $z0
+ STR_ZXI $z0, $x0, 0 :: (store (<vscale x 1 x s128>))
+ BUNDLE implicit-def $z1, implicit killed $z0 {
+ $z1 = ADD_ZZZ_D killed $z0, $z0
+ }
+ STR_ZXI renamable $z1, $x0, 1 :: (store (<vscale x 1 x s128>))
+ RET_ReallyLR
+...
+# CHECK-LABEL: name: clear-kill-in-bundle-forward
+# CHECK: BUNDLE implicit-def $z1, implicit $z0 {
+# CHECK: $z1 = ADD_ZZZ_D $z0, $z0
+# CHECK: }
+# CHECK: STPQi $q0, $q1, $x0, 0 :: (store (<vscale x 1 x s128>))
|
arsenm
reviewed
Aug 11, 2025
| @@ -1193,7 +1193,8 @@ AArch64LoadStoreOpt::mergePairedInsns(MachineBasicBlock::iterator I, | |||
| // USE kill %w1 ; need to clear kill flag when moving STRWui downwards | |||
| // STRW %w0 | |||
| Register Reg = getLdStRegOp(*I).getReg(); | |||
| for (MachineInstr &MI : make_range(std::next(I), Paired)) | |||
| for (MachineInstr &MI : | |||
There was a problem hiding this comment.
Can just do MRI.clearKillFlags
There was a problem hiding this comment.
I think the kill flags of I should be preserved here as they are used to track defined registers.
There was a problem hiding this comment.
Kill flags have been deprecated for over a decade and should not be relied on. We should be be actively purging uses and not bothering to preserve them. Liveness should be tracked by reverse walking from the bottom of the block which avoids the need for them
There was a problem hiding this comment.
Sorry, what I was trying to say is that the code, as written, currently depends on some kill flags being preserved. If we use MRI.clearKillFlags here without further refactoring the code, we'll have miscompiles.
There was a problem hiding this comment.
Missed kill flags should at worst give missed optimizations, they could never be relied on for correctness
There was a problem hiding this comment.
With MRI.clearKillFlags, test12 from 17554b8, which added the bit of code I linked previously, miscompiles due to a register being incorrectly renamed:
Function Live Ins: $x0, $x1, $x8
bb.0:
liveins: $x0, $x1
renamable $x10 = LDRXui renamable $x0, 0 :: (load (s64))
$x10, renamable $x8 = LDPXi renamable $x0, 3 :: (load (s64))
renamable $x9 = LDRXui renamable $x0, 2 :: (load (s64))
renamable $x8 = ADDXrr $x8, $x8
STPXi renamable $x8, killed $x10, renamable $x0, 10 :: (store (s64), align 4)
STPXi renamable $x10, renamable $x9, renamable $x0, 20 :: (store (s64), align 4)
RET undef $lr
As you say, this could be done differently, but doing MRI.clearKillFlags doesn't seem correct without first refactoring the code (which I believe would be preferably done separately).
davemgreen
approved these changes
Aug 12, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/22475 Here is the relevant piece of the build log for the reference |
This was referenced Aug 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As an alternative to #149177, iterate through all instructions in
AArch64LoadStoreOptimizer.