-
Notifications
You must be signed in to change notification settings - Fork 15.2k
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.
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
MachineCombiner: Partially fix losing subregister indexes #141661
Conversation
| continue; | ||
| if (Idx == RootFirstOpIdx) | ||
| MIB2 = MIB2.addReg(RegA, getKillRegState(KillA)); | ||
| MIB2 = MIB2.addReg(RegA, getKillRegState(KillA), SubRegA); |
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.
I think you might be missing a swap in the SwapRootOperands just above. Not sure what a sensible subreg is for NewVR is though.
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.
It's used as a result register, and this is still SSA so it cannot have a subregister
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.
Right, but we swapped NewVR and RegA above, and the SubRegA here wasn't swapped.
84a4aea to
6b1e209
Compare
|
ping |
4 similar comments
|
ping |
|
ping |
|
ping |
|
ping |
This fixes verifier errors in this test after earlier passes start introducing more subregister uses. This probably isn't adequately tested but I know nothing about this pass.
6b1e209 to
baeb410
Compare
|
@llvm/pr-subscribers-backend-aarch64 Author: Matt Arsenault (arsenm) ChangesThis fixes verifier errors in this test after earlier passes start Full diff: https://github.com/llvm/llvm-project/pull/141661.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 76f99848a823c..d503d7a2345fd 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -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())
@@ -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);
@@ -1362,6 +1366,7 @@ void TargetInstrInfo::reassociateOps(
if (SwapPrevOperands) {
std::swap(RegX, RegY);
+ std::swap(SubRegX, SubRegY);
std::swap(KillX, KillY);
}
@@ -1414,9 +1419,9 @@ 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);
}
@@ -1424,6 +1429,7 @@ void TargetInstrInfo::reassociateOps(
if (SwapRootOperands) {
std::swap(RegA, NewVR);
+ std::swap(SubRegA, SubRegNewVR);
std::swap(KillA, KillNewVR);
}
@@ -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);
else if (Idx == RootSecondOpIdx)
- MIB2 = MIB2.addReg(NewVR, getKillRegState(KillNewVR));
+ MIB2 = MIB2.addReg(NewVR, getKillRegState(KillNewVR), SubRegNewVR);
else
MIB2 = MIB2.add(MO);
}
@@ -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;
diff --git a/llvm/test/CodeGen/AArch64/machine-combiner-subregs.mir b/llvm/test/CodeGen/AArch64/machine-combiner-subregs.mir
new file mode 100644
index 0000000000000..c96a0385c3a4e
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/machine-combiner-subregs.mir
@@ -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
+
+...
diff --git a/llvm/test/CodeGen/RISCV/rvv/machine-combiner-subreg-verifier-error.mir b/llvm/test/CodeGen/RISCV/rvv/machine-combiner-subreg-verifier-error.mir
new file mode 100644
index 0000000000000..76dfd4e746bea
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/rvv/machine-combiner-subreg-verifier-error.mir
@@ -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
+
+...
|
|
@llvm/pr-subscribers-backend-risc-v Author: Matt Arsenault (arsenm) ChangesThis fixes verifier errors in this test after earlier passes start Full diff: https://github.com/llvm/llvm-project/pull/141661.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 76f99848a823c..d503d7a2345fd 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -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())
@@ -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);
@@ -1362,6 +1366,7 @@ void TargetInstrInfo::reassociateOps(
if (SwapPrevOperands) {
std::swap(RegX, RegY);
+ std::swap(SubRegX, SubRegY);
std::swap(KillX, KillY);
}
@@ -1414,9 +1419,9 @@ 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);
}
@@ -1424,6 +1429,7 @@ void TargetInstrInfo::reassociateOps(
if (SwapRootOperands) {
std::swap(RegA, NewVR);
+ std::swap(SubRegA, SubRegNewVR);
std::swap(KillA, KillNewVR);
}
@@ -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);
else if (Idx == RootSecondOpIdx)
- MIB2 = MIB2.addReg(NewVR, getKillRegState(KillNewVR));
+ MIB2 = MIB2.addReg(NewVR, getKillRegState(KillNewVR), SubRegNewVR);
else
MIB2 = MIB2.add(MO);
}
@@ -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;
diff --git a/llvm/test/CodeGen/AArch64/machine-combiner-subregs.mir b/llvm/test/CodeGen/AArch64/machine-combiner-subregs.mir
new file mode 100644
index 0000000000000..c96a0385c3a4e
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/machine-combiner-subregs.mir
@@ -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
+
+...
diff --git a/llvm/test/CodeGen/RISCV/rvv/machine-combiner-subreg-verifier-error.mir b/llvm/test/CodeGen/RISCV/rvv/machine-combiner-subreg-verifier-error.mir
new file mode 100644
index 0000000000000..76dfd4e746bea
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/rvv/machine-combiner-subreg-verifier-error.mir
@@ -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
+
+...
|
|
Added test. I tried to test and handle the broken accumulator part, but the existing tests all seem to use instructions with large registers that aren't available as subregisters |
topperc
left a comment
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

This fixes verifier errors in this test after earlier passes start
introducing more subregister uses. This probably isn't adequately
tested but I know nothing about this pass.