Skip to content

MachineCombiner: Partially fix losing subregister indexes #141661

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 3 commits into from
Nov 11, 2025
Merged

MachineCombiner: Partially fix losing subregister indexes #141661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42e0402
MachineCombiner: Partially fix losing subregister indexes
arsenm May 27, 2025
c2163dc
Try to track NewVR subreg through swap
arsenm Jun 25, 2025
baeb410
Add test
arsenm Nov 11, 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
15 changes: 11 additions & 4 deletions llvm/lib/CodeGen/TargetInstrInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,9 +1330,12 @@ void TargetInstrInfo::reassociateOps(
MachineOperand &OpC = Root.getOperand(0);

Register RegA = OpA.getReg();
unsigned SubRegA = OpA.getSubReg();
Register RegB = OpB.getReg();
Register RegX = OpX.getReg();
unsigned SubRegX = OpX.getSubReg();
Register RegY = OpY.getReg();
unsigned SubRegY = OpY.getSubReg();
Register RegC = OpC.getReg();

if (RegA.isVirtual())
Expand All @@ -1350,6 +1353,7 @@ void TargetInstrInfo::reassociateOps(
// recycling RegB because the MachineCombiner's computation of the critical
// path requires a new register definition rather than an existing one.
Register NewVR = MRI.createVirtualRegister(RC);
unsigned SubRegNewVR = 0;
InstrIdxForVirtReg.insert(std::make_pair(NewVR, 0));

auto [NewRootOpc, NewPrevOpc] = getReassociationOpcodes(Pattern, Root, Prev);
Expand All @@ -1362,6 +1366,7 @@ void TargetInstrInfo::reassociateOps(

if (SwapPrevOperands) {
std::swap(RegX, RegY);
std::swap(SubRegX, SubRegY);
std::swap(KillX, KillY);
}

Expand Down Expand Up @@ -1414,16 +1419,17 @@ void TargetInstrInfo::reassociateOps(
if (Idx == 0)
continue;
if (Idx == PrevFirstOpIdx)
MIB1.addReg(RegX, getKillRegState(KillX));
MIB1.addReg(RegX, getKillRegState(KillX), SubRegX);
else if (Idx == PrevSecondOpIdx)
MIB1.addReg(RegY, getKillRegState(KillY));
MIB1.addReg(RegY, getKillRegState(KillY), SubRegY);
else
MIB1.add(MO);
}
MIB1.copyImplicitOps(Prev);

if (SwapRootOperands) {
std::swap(RegA, NewVR);
std::swap(SubRegA, SubRegNewVR);
std::swap(KillA, KillNewVR);
}

Expand All @@ -1435,9 +1441,9 @@ void TargetInstrInfo::reassociateOps(
if (Idx == 0)
continue;
if (Idx == RootFirstOpIdx)
MIB2 = MIB2.addReg(RegA, getKillRegState(KillA));
MIB2 = MIB2.addReg(RegA, getKillRegState(KillA), SubRegA);
Copy link
Collaborator

@preames preames May 28, 2025

Choose a reason for hiding this comment

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

I think you might be missing a swap in the SwapRootOperands just above. Not sure what a sensible subreg is for NewVR is though.

Copy link
Contributor Author

@arsenm arsenm May 28, 2025

Choose a reason for hiding this comment

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

It's used as a result register, and this is still SSA so it cannot have a subregister

Copy link
Collaborator

@preames preames May 28, 2025

Choose a reason for hiding this comment

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

Right, but we swapped NewVR and RegA above, and the SubRegA here wasn't swapped.

else if (Idx == RootSecondOpIdx)
MIB2 = MIB2.addReg(NewVR, getKillRegState(KillNewVR));
MIB2 = MIB2.addReg(NewVR, getKillRegState(KillNewVR), SubRegNewVR);
else
MIB2 = MIB2.add(MO);
}
Expand Down Expand Up @@ -1525,6 +1531,7 @@ void TargetInstrInfo::genAlternativeCodeSequence(
if (IndexedReg.index() == 0)
continue;

// FIXME: Losing subregisters
MachineInstr *Instr = MRI.getUniqueVRegDef(IndexedReg.value());
MachineInstrBuilder MIB;
Register AccReg;
Expand Down
35 changes: 35 additions & 0 deletions llvm/test/CodeGen/AArch64/machine-combiner-subregs.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 6
# RUN: llc -mtriple=aarch64-gnu-linux -mcpu=neoverse-n2 -run-pass=machine-combiner -o - %s | FileCheck %s

# Make sure machine combiner doesn't drop subregister indexes.

---
name: reassociate_adds2_reassoc
tracksRegLiveness: true
body: |
bb.0:
liveins: $q0, $q1, $q2, $q3

; CHECK-LABEL: name: reassociate_adds2_reassoc
; CHECK: liveins: $q0, $q1, $q2, $q3
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:fpr128 = COPY $q0
; CHECK-NEXT: [[COPY1:%[0-9]+]]:fpr128 = COPY $q1
; CHECK-NEXT: [[COPY2:%[0-9]+]]:fpr128 = COPY $q2
; CHECK-NEXT: [[COPY3:%[0-9]+]]:fpr128 = COPY $q3
; CHECK-NEXT: [[FADDSrr:%[0-9]+]]:fpr32 = nsz reassoc nofpexcept FADDSrr [[COPY]].ssub, [[COPY1]].ssub, implicit $fpcr
; CHECK-NEXT: [[FADDSrr1:%[0-9]+]]:fpr32 = nsz reassoc nofpexcept FADDSrr [[COPY2]].ssub, [[COPY3]].ssub, implicit $fpcr
; CHECK-NEXT: [[FADDSrr2:%[0-9]+]]:fpr32 = nsz reassoc nofpexcept FADDSrr killed [[FADDSrr1]], killed [[FADDSrr]], implicit $fpcr
; CHECK-NEXT: $s0 = COPY [[FADDSrr2]]
; CHECK-NEXT: RET_ReallyLR implicit $s0
%0:fpr128 = COPY $q0
%1:fpr128 = COPY $q1
%2:fpr128 = COPY $q2
%3:fpr128 = COPY $q3
%4:fpr32 = nsz reassoc nofpexcept FADDSrr %0.ssub, %1.ssub, implicit $fpcr
%5:fpr32 = nsz reassoc nofpexcept FADDSrr %2.ssub, killed %4, implicit $fpcr
%6:fpr32 = nsz reassoc nofpexcept FADDSrr killed %5, %3.ssub, implicit $fpcr
$s0 = COPY %6
RET_ReallyLR implicit $s0

...
39 changes: 39 additions & 0 deletions llvm/test/CodeGen/RISCV/rvv/machine-combiner-subreg-verifier-error.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
# RUN: llc -mtriple=riscv64 -mattr=+v -verify-machineinstrs -run-pass=machine-combiner -o - %s | FileCheck %s

# Make sure the verifier doesn't fail due to dropping subregister
# uses.

---
name: machine_combiner_subreg_verifier_error
tracksRegLiveness: true
isSSA: true
body: |
bb.0:
liveins: $v8m4, $v12m4

; CHECK-LABEL: name: machine_combiner_subreg_verifier_error
; CHECK: liveins: $v8m4, $v12m4
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[DEF:%[0-9]+]]:vrm4 = IMPLICIT_DEF
; CHECK-NEXT: [[DEF1:%[0-9]+]]:gprnox0 = IMPLICIT_DEF
; CHECK-NEXT: [[DEF2:%[0-9]+]]:vrm8 = IMPLICIT_DEF
; CHECK-NEXT: [[DEF3:%[0-9]+]]:vr = IMPLICIT_DEF
; CHECK-NEXT: [[DEF4:%[0-9]+]]:vrm2 = IMPLICIT_DEF
; CHECK-NEXT: [[DEF5:%[0-9]+]]:vr = IMPLICIT_DEF
; CHECK-NEXT: [[PseudoVSLIDEDOWN_VI_M8_:%[0-9]+]]:vrm8 = PseudoVSLIDEDOWN_VI_M8 $noreg, [[DEF2]], 26, 2, 5 /* e32 */, 3 /* ta, ma */
; CHECK-NEXT: [[PseudoVADD_VV_MF2_:%[0-9]+]]:vr = PseudoVADD_VV_MF2 $noreg, [[DEF2]].sub_vrm1_0, killed [[DEF3]], 2, 5 /* e32 */, 1 /* ta, mu */
; CHECK-NEXT: [[PseudoVADD_VV_MF2_1:%[0-9]+]]:vr = PseudoVADD_VV_MF2 $noreg, [[PseudoVSLIDEDOWN_VI_M8_]].sub_vrm1_0, killed [[PseudoVADD_VV_MF2_]], 2, 5 /* e32 */, 1 /* ta, mu */
; CHECK-NEXT: PseudoRET implicit $v8
%0:vrm4 = IMPLICIT_DEF
%1:gprnox0 = IMPLICIT_DEF
%2:vrm8 = IMPLICIT_DEF
%3:vr = IMPLICIT_DEF
%4:vrm2 = IMPLICIT_DEF
%5:vr = IMPLICIT_DEF
%6:vrm8 = PseudoVSLIDEDOWN_VI_M8 $noreg, %2, 26, 2, 5 /* e32 */, 3 /* ta, ma */
%7:vr = PseudoVADD_VV_MF2 $noreg, %6.sub_vrm1_0, %2.sub_vrm1_0, 2, 5 /* e32 */, 1 /* ta, mu */
%8:vr = PseudoVADD_VV_MF2 $noreg, killed %7, killed %3, 2, 5 /* e32 */, 1 /* ta, mu */
PseudoRET implicit $v8

...
Loading