Skip to content

[RISCV][MoveMerge] Don't copy kill flag when moving past an instruction that reads the register. #153644

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 2 commits into from
Aug 14, 2025
Merged

[RISCV][MoveMerge] Don't copy kill flag when moving past an instruction that reads the register. #153644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a67ce72
[RISCV][MoveMerge] Don't copy kill flag when moving past an instructi…
topperc Aug 14, 2025
80b0497
fixup! commit test
topperc Aug 14, 2025
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
16 changes: 14 additions & 2 deletions llvm/lib/Target/RISCV/RISCVMoveMerger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,11 @@ RISCVMoveMerge::mergePairedInsns(MachineBasicBlock::iterator I,
NextI = next_nodbg(NextI, E);
DebugLoc DL = I->getDebugLoc();

// Make a copy so we can update the kill flag in the MoveFromAToS case. The
// copied operand needs to be scoped outside the if since we make a pointer
// to it.
MachineOperand PairedSource = *PairedRegs.Source;

// The order of S-reg depends on which instruction holds A0, instead of
// the order of register pair.
// e,g.
Expand All @@ -147,8 +152,15 @@ RISCVMoveMerge::mergePairedInsns(MachineBasicBlock::iterator I,
// mv a1, s1 => cm.mva01s s2,s1
bool StartWithX10 = ARegInFirstPair == RISCV::X10;
if (isMoveFromAToS(Opcode)) {
Sreg1 = StartWithX10 ? FirstPair.Source : PairedRegs.Source;
Sreg2 = StartWithX10 ? PairedRegs.Source : FirstPair.Source;
// We are moving one of the copies earlier so its kill flag may become
// invalid. Clear the copied kill flag if there are any reads of the
// register between the new location and the old location.
for (auto It = std::next(I); It != Paired && PairedSource.isKill(); ++It)
if (It->readsRegister(PairedSource.getReg(), TRI))
PairedSource.setIsKill(false);

Sreg1 = StartWithX10 ? FirstPair.Source : &PairedSource;
Sreg2 = StartWithX10 ? &PairedSource : FirstPair.Source;
} else {
Sreg1 = StartWithX10 ? FirstPair.Destination : PairedRegs.Destination;
Sreg2 = StartWithX10 ? PairedRegs.Destination : FirstPair.Destination;
Comment on lines 165 to 166
Copy link
Member

@lenary lenary Aug 14, 2025

Choose a reason for hiding this comment

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

I don't understand why this case doesn't move a copy earlier, can you explain that further?

Copy link
Collaborator Author

@topperc topperc Aug 14, 2025

Choose a reason for hiding this comment

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

It does but we don’t copy the source operands. They’re implicit uses on the new instruction.

Copy link
Member

@lenary lenary Aug 14, 2025

Choose a reason for hiding this comment

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

And the key thing is the implicit uses are not marked as a kill. Cool.

Expand Down
23 changes: 23 additions & 0 deletions llvm/test/CodeGen/RISCV/pr153598.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
# RUN: llc -mtriple=riscv32 -mattr=+zcmp -run-pass=riscv-move-merge -verify-machineinstrs %s -o - | FileCheck %s
---
name: mov-merge
tracksRegLiveness: true
body: |
bb.0.entry:
liveins: $x8, $x9
; CHECK-LABEL: name: mov-merge
; CHECK: liveins: $x8, $x9
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: $x12 = ADDI $x0, -3
; CHECK-NEXT: SW renamable $x9, $x2, 56
; CHECK-NEXT: CM_MVA01S killed renamable $x9, renamable $x8, implicit-def $x10, implicit-def $x11
; CHECK-NEXT: SW renamable $x8, $x2, 60
; CHECK-NEXT: PseudoRET
$x12 = ADDI $x0, -3
SW renamable $x9, $x2, 56
$x10 = ADDI killed renamable $x9, 0
SW renamable $x8, $x2, 60
$x11 = ADDI killed renamable $x8, 0
PseudoRET
...
Loading