From 42e04023f86a842e2f2c5ed1eaa683504ef671a8 Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Tue, 27 May 2025 21:09:07 +0200 Subject: [PATCH 1/3] MachineCombiner: Partially fix losing subregister indexes 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. --- llvm/lib/CodeGen/TargetInstrInfo.cpp | 11 ++++-- ...machine-combiner-subreg-verifier-error.mir | 39 +++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 llvm/test/CodeGen/RISCV/rvv/machine-combiner-subreg-verifier-error.mir diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp index 76f99848a823c..d0546f700daab 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()) @@ -1362,6 +1365,7 @@ void TargetInstrInfo::reassociateOps( if (SwapPrevOperands) { std::swap(RegX, RegY); + std::swap(SubRegX, SubRegY); std::swap(KillX, KillY); } @@ -1414,9 +1418,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); } @@ -1435,7 +1439,7 @@ 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)); else @@ -1525,6 +1529,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/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 + +... From c2163dc7d1b31ee32e531c5c949fb51bcade74ba Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Thu, 26 Jun 2025 07:54:46 +0900 Subject: [PATCH 2/3] Try to track NewVR subreg through swap --- llvm/lib/CodeGen/TargetInstrInfo.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp index d0546f700daab..d503d7a2345fd 100644 --- a/llvm/lib/CodeGen/TargetInstrInfo.cpp +++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp @@ -1353,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); @@ -1428,6 +1429,7 @@ void TargetInstrInfo::reassociateOps( if (SwapRootOperands) { std::swap(RegA, NewVR); + std::swap(SubRegA, SubRegNewVR); std::swap(KillA, KillNewVR); } @@ -1441,7 +1443,7 @@ void TargetInstrInfo::reassociateOps( if (Idx == RootFirstOpIdx) 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); } From baeb4108d56325610382243ab05a10a510940ddc Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Mon, 10 Nov 2025 16:22:03 -0800 Subject: [PATCH 3/3] Add test --- .../AArch64/machine-combiner-subregs.mir | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 llvm/test/CodeGen/AArch64/machine-combiner-subregs.mir 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 + +...