Skip to content

[AArch64] Fix stp kill when merging forward. #152994

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 12, 2025
Merged

[AArch64] Fix stp kill when merging forward. #152994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can just do MRI.clearKillFlags

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the kill flags of I should be preserved here as they are used to track defined registers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed kill flags should at worst give missed optimizations, they could never be relied on for correctness

Copy link
Contributor Author

@rj-jesus rj-jesus Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

make_range(std::next(I->getIterator()), Paired->getIterator()))
MI.clearRegisterKills(Reg, TRI);
}
}
Expand Down
18 changes: 18 additions & 0 deletions llvm/test/CodeGen/AArch64/sve-vls-ldst-opt.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>))