From 03ecc8df8e5843474901689a82450e0f32e57701 Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Thu, 23 Jan 2025 16:34:03 +0700 Subject: [PATCH 1/3] X86: Fix convertToThreeAddress losing subregister indexes This avoids dozens of regressions in a future patch. These primarily manifested as assertions where we had copies of 64-bit registers to 32-bit registers. This is testable in principle with hand written MIR, but that's a bit too much x86 for me. --- llvm/lib/Target/X86/X86InstrInfo.cpp | 77 ++++++++++++++++------------ llvm/lib/Target/X86/X86InstrInfo.h | 5 +- 2 files changed, 47 insertions(+), 35 deletions(-) diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp b/llvm/lib/Target/X86/X86InstrInfo.cpp index 44db5b6865c422..3750ed194c4651 100644 --- a/llvm/lib/Target/X86/X86InstrInfo.cpp +++ b/llvm/lib/Target/X86/X86InstrInfo.cpp @@ -1158,8 +1158,9 @@ static bool findRedundantFlagInstr(MachineInstr &CmpInstr, bool X86InstrInfo::classifyLEAReg(MachineInstr &MI, const MachineOperand &Src, unsigned Opc, bool AllowSP, Register &NewSrc, - bool &isKill, MachineOperand &ImplicitOp, - LiveVariables *LV, LiveIntervals *LIS) const { + unsigned &NewSrcSubReg, bool &isKill, + MachineOperand &ImplicitOp, LiveVariables *LV, + LiveIntervals *LIS) const { MachineFunction &MF = *MI.getParent()->getParent(); const TargetRegisterClass *RC; if (AllowSP) { @@ -1168,12 +1169,14 @@ bool X86InstrInfo::classifyLEAReg(MachineInstr &MI, const MachineOperand &Src, RC = Opc != X86::LEA32r ? &X86::GR64_NOSPRegClass : &X86::GR32_NOSPRegClass; } Register SrcReg = Src.getReg(); + unsigned SubReg = Src.getSubReg(); isKill = MI.killsRegister(SrcReg, /*TRI=*/nullptr); // For both LEA64 and LEA32 the register already has essentially the right // type (32-bit or 64-bit) we may just need to forbid SP. if (Opc != X86::LEA64_32r) { NewSrc = SrcReg; + NewSrcSubReg = SubReg; assert(!Src.isUndef() && "Undef op doesn't need optimization"); if (NewSrc.isVirtual() && !MF.getRegInfo().constrainRegClass(NewSrc, RC)) @@ -1189,6 +1192,7 @@ bool X86InstrInfo::classifyLEAReg(MachineInstr &MI, const MachineOperand &Src, ImplicitOp.setImplicit(); NewSrc = getX86SubSuperRegister(SrcReg, 64); + assert(!SubReg && "no superregister for source"); assert(NewSrc.isValid() && "Invalid Operand"); assert(!Src.isUndef() && "Undef op doesn't need optimization"); } else { @@ -1198,7 +1202,7 @@ bool X86InstrInfo::classifyLEAReg(MachineInstr &MI, const MachineOperand &Src, MachineInstr *Copy = BuildMI(*MI.getParent(), MI, MI.getDebugLoc(), get(TargetOpcode::COPY)) .addReg(NewSrc, RegState::Define | RegState::Undef, X86::sub_32bit) - .addReg(SrcReg, getKillRegState(isKill)); + .addReg(SrcReg, getKillRegState(isKill), SubReg); // Which is obviously going to be dead after we're done with it. isKill = true; @@ -1258,7 +1262,9 @@ MachineInstr *X86InstrInfo::convertToThreeAddressWithLEA(unsigned MIOpc, MachineBasicBlock::iterator MBBI = MI.getIterator(); Register Dest = MI.getOperand(0).getReg(); Register Src = MI.getOperand(1).getReg(); + unsigned SrcSubReg = MI.getOperand(1).getSubReg(); Register Src2; + unsigned Src2SubReg; bool IsDead = MI.getOperand(0).isDead(); bool IsKill = MI.getOperand(1).isKill(); unsigned SubReg = Is8BitOp ? X86::sub_8bit : X86::sub_16bit; @@ -1268,7 +1274,7 @@ MachineInstr *X86InstrInfo::convertToThreeAddressWithLEA(unsigned MIOpc, MachineInstr *InsMI = BuildMI(MBB, MBBI, MI.getDebugLoc(), get(TargetOpcode::COPY)) .addReg(InRegLEA, RegState::Define, SubReg) - .addReg(Src, getKillRegState(IsKill)); + .addReg(Src, getKillRegState(IsKill), SrcSubReg); MachineInstr *ImpDef2 = nullptr; MachineInstr *InsMI2 = nullptr; @@ -1306,6 +1312,7 @@ MachineInstr *X86InstrInfo::convertToThreeAddressWithLEA(unsigned MIOpc, case X86::ADD16rr: case X86::ADD16rr_DB: { Src2 = MI.getOperand(2).getReg(); + Src2SubReg = MI.getOperand(2).getSubReg(); bool IsKill2 = MI.getOperand(2).isKill(); assert(!MI.getOperand(2).isUndef() && "Undef op doesn't need optimization"); if (Src == Src2) { @@ -1323,7 +1330,7 @@ MachineInstr *X86InstrInfo::convertToThreeAddressWithLEA(unsigned MIOpc, InRegLEA2); InsMI2 = BuildMI(MBB, &*MIB, MI.getDebugLoc(), get(TargetOpcode::COPY)) .addReg(InRegLEA2, RegState::Define, SubReg) - .addReg(Src2, getKillRegState(IsKill2)); + .addReg(Src2, getKillRegState(IsKill2), Src2SubReg); addRegReg(MIB, InRegLEA, true, InRegLEA2, true); } if (LV && IsKill2 && InsMI2) @@ -1428,6 +1435,7 @@ MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI, MachineInstr *NewMI = nullptr; Register SrcReg, SrcReg2; + unsigned SrcSubReg, SrcSubReg2; bool Is64Bit = Subtarget.is64Bit(); bool Is8BitOp = false; @@ -1467,17 +1475,18 @@ MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI, // LEA can't handle ESP. bool isKill; MachineOperand ImplicitOp = MachineOperand::CreateReg(0, false); - if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/false, SrcReg, isKill, - ImplicitOp, LV, LIS)) + if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/false, SrcReg, SrcSubReg, + isKill, ImplicitOp, LV, LIS)) return nullptr; - MachineInstrBuilder MIB = BuildMI(MF, MI.getDebugLoc(), get(Opc)) - .add(Dest) - .addReg(0) - .addImm(1LL << ShAmt) - .addReg(SrcReg, getKillRegState(isKill)) - .addImm(0) - .addReg(0); + MachineInstrBuilder MIB = + BuildMI(MF, MI.getDebugLoc(), get(Opc)) + .add(Dest) + .addReg(0) + .addImm(1LL << ShAmt) + .addReg(SrcReg, getKillRegState(isKill), SrcSubReg) + .addImm(0) + .addReg(0); if (ImplicitOp.getReg() != 0) MIB.add(ImplicitOp); NewMI = MIB; @@ -1505,8 +1514,8 @@ MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI, : (Is64Bit ? X86::LEA64_32r : X86::LEA32r); bool isKill; MachineOperand ImplicitOp = MachineOperand::CreateReg(0, false); - if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/false, SrcReg, isKill, - ImplicitOp, LV, LIS)) + if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/false, SrcReg, SrcSubReg, + isKill, ImplicitOp, LV, LIS)) return nullptr; MachineInstrBuilder MIB = BuildMI(MF, MI.getDebugLoc(), get(Opc)) @@ -1531,8 +1540,8 @@ MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI, bool isKill; MachineOperand ImplicitOp = MachineOperand::CreateReg(0, false); - if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/false, SrcReg, isKill, - ImplicitOp, LV, LIS)) + if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/false, SrcReg, SrcSubReg, + isKill, ImplicitOp, LV, LIS)) return nullptr; MachineInstrBuilder MIB = BuildMI(MF, MI.getDebugLoc(), get(Opc)) @@ -1569,8 +1578,8 @@ MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI, const MachineOperand &Src2 = MI.getOperand(2); bool isKill2; MachineOperand ImplicitOp2 = MachineOperand::CreateReg(0, false); - if (!classifyLEAReg(MI, Src2, Opc, /*AllowSP=*/false, SrcReg2, isKill2, - ImplicitOp2, LV, LIS)) + if (!classifyLEAReg(MI, Src2, Opc, /*AllowSP=*/false, SrcReg2, SrcSubReg2, + isKill2, ImplicitOp2, LV, LIS)) return nullptr; bool isKill; @@ -1581,8 +1590,8 @@ MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI, isKill = isKill2; SrcReg = SrcReg2; } else { - if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/true, SrcReg, isKill, - ImplicitOp, LV, LIS)) + if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/true, SrcReg, SrcSubReg, + isKill, ImplicitOp, LV, LIS)) return nullptr; } @@ -1592,7 +1601,7 @@ MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI, if (ImplicitOp2.getReg() != 0) MIB.add(ImplicitOp2); - NewMI = addRegReg(MIB, SrcReg, isKill, SrcReg2, isKill2); + NewMI = addRegReg(MIB, SrcReg, isKill, SrcReg2, isKill2); // FIXME: Subregs // Add kills if classifyLEAReg created a new register. if (LV) { @@ -1625,13 +1634,14 @@ MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI, bool isKill; MachineOperand ImplicitOp = MachineOperand::CreateReg(0, false); - if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/true, SrcReg, isKill, - ImplicitOp, LV, LIS)) + if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/true, SrcReg, SrcSubReg, + isKill, ImplicitOp, LV, LIS)) return nullptr; - MachineInstrBuilder MIB = BuildMI(MF, MI.getDebugLoc(), get(Opc)) - .add(Dest) - .addReg(SrcReg, getKillRegState(isKill)); + MachineInstrBuilder MIB = + BuildMI(MF, MI.getDebugLoc(), get(Opc)) + .add(Dest) + .addReg(SrcReg, getKillRegState(isKill), SrcSubReg); if (ImplicitOp.getReg() != 0) MIB.add(ImplicitOp); @@ -1665,13 +1675,14 @@ MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI, bool isKill; MachineOperand ImplicitOp = MachineOperand::CreateReg(0, false); - if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/true, SrcReg, isKill, - ImplicitOp, LV, LIS)) + if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/true, SrcReg, SrcSubReg, + isKill, ImplicitOp, LV, LIS)) return nullptr; - MachineInstrBuilder MIB = BuildMI(MF, MI.getDebugLoc(), get(Opc)) - .add(Dest) - .addReg(SrcReg, getKillRegState(isKill)); + MachineInstrBuilder MIB = + BuildMI(MF, MI.getDebugLoc(), get(Opc)) + .add(Dest) + .addReg(SrcReg, getKillRegState(isKill), SrcSubReg); if (ImplicitOp.getReg() != 0) MIB.add(ImplicitOp); diff --git a/llvm/lib/Target/X86/X86InstrInfo.h b/llvm/lib/Target/X86/X86InstrInfo.h index 5f87e02fe67c41..e499f925f48ec3 100644 --- a/llvm/lib/Target/X86/X86InstrInfo.h +++ b/llvm/lib/Target/X86/X86InstrInfo.h @@ -310,8 +310,9 @@ class X86InstrInfo final : public X86GenInstrInfo { /// operand to the LEA instruction. bool classifyLEAReg(MachineInstr &MI, const MachineOperand &Src, unsigned LEAOpcode, bool AllowSP, Register &NewSrc, - bool &isKill, MachineOperand &ImplicitOp, - LiveVariables *LV, LiveIntervals *LIS) const; + unsigned &NewSrcSubReg, bool &isKill, + MachineOperand &ImplicitOp, LiveVariables *LV, + LiveIntervals *LIS) const; /// convertToThreeAddress - This method must be implemented by targets that /// set the M_CONVERTIBLE_TO_3_ADDR flag. When this flag is set, the target From 065046bf6fa8f69587d323e8b62421abb8d396a5 Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Tue, 18 Feb 2025 22:27:51 +0700 Subject: [PATCH 2/3] Address addRegReg fixme --- llvm/lib/Target/X86/X86FastISel.cpp | 3 ++- llvm/lib/Target/X86/X86InstrBuilder.h | 13 ++++++++----- llvm/lib/Target/X86/X86InstrInfo.cpp | 10 +++++++--- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/llvm/lib/Target/X86/X86FastISel.cpp b/llvm/lib/Target/X86/X86FastISel.cpp index 039b8929c93a8e..23de30275e2a19 100644 --- a/llvm/lib/Target/X86/X86FastISel.cpp +++ b/llvm/lib/Target/X86/X86FastISel.cpp @@ -3830,7 +3830,8 @@ unsigned X86FastISel::X86MaterializeFP(const ConstantFP *CFP, MVT VT) { .addConstantPoolIndex(CPI, 0, OpFlag); MachineInstrBuilder MIB = BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD, TII.get(Opc), ResultReg); - addRegReg(MIB, AddrReg, false, PICBase, false); + addRegReg(MIB, AddrReg, false, X86::NoSubRegister, PICBase, false, + X86::NoSubRegister); MachineMemOperand *MMO = FuncInfo.MF->getMachineMemOperand( MachinePointerInfo::getConstantPool(*FuncInfo.MF), MachineMemOperand::MOLoad, DL.getPointerSize(), Alignment); diff --git a/llvm/lib/Target/X86/X86InstrBuilder.h b/llvm/lib/Target/X86/X86InstrBuilder.h index 07079ef87fd464..45c5f8aa82e978 100644 --- a/llvm/lib/Target/X86/X86InstrBuilder.h +++ b/llvm/lib/Target/X86/X86InstrBuilder.h @@ -161,11 +161,14 @@ addRegOffset(const MachineInstrBuilder &MIB, /// addRegReg - This function is used to add a memory reference of the form: /// [Reg + Reg]. -static inline const MachineInstrBuilder &addRegReg(const MachineInstrBuilder &MIB, - unsigned Reg1, bool isKill1, - unsigned Reg2, bool isKill2) { - return MIB.addReg(Reg1, getKillRegState(isKill1)).addImm(1) - .addReg(Reg2, getKillRegState(isKill2)).addImm(0).addReg(0); +static inline const MachineInstrBuilder & +addRegReg(const MachineInstrBuilder &MIB, unsigned Reg1, bool isKill1, + unsigned SubReg1, unsigned Reg2, bool isKill2, unsigned SubReg2) { + return MIB.addReg(Reg1, getKillRegState(isKill1), SubReg1) + .addImm(1) + .addReg(Reg2, getKillRegState(isKill2), SubReg2) + .addImm(0) + .addReg(0); } static inline const MachineInstrBuilder & diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp b/llvm/lib/Target/X86/X86InstrInfo.cpp index 3750ed194c4651..037fce3d3a344c 100644 --- a/llvm/lib/Target/X86/X86InstrInfo.cpp +++ b/llvm/lib/Target/X86/X86InstrInfo.cpp @@ -1318,7 +1318,8 @@ MachineInstr *X86InstrInfo::convertToThreeAddressWithLEA(unsigned MIOpc, if (Src == Src2) { // ADD8rr/ADD16rr killed %reg1028, %reg1028 // just a single insert_subreg. - addRegReg(MIB, InRegLEA, true, InRegLEA, false); + addRegReg(MIB, InRegLEA, true, X86::NoSubRegister, InRegLEA, false, + X86::NoSubRegister); } else { if (Subtarget.is64Bit()) InRegLEA2 = RegInfo.createVirtualRegister(&X86::GR64_NOSPRegClass); @@ -1331,7 +1332,8 @@ MachineInstr *X86InstrInfo::convertToThreeAddressWithLEA(unsigned MIOpc, InsMI2 = BuildMI(MBB, &*MIB, MI.getDebugLoc(), get(TargetOpcode::COPY)) .addReg(InRegLEA2, RegState::Define, SubReg) .addReg(Src2, getKillRegState(IsKill2), Src2SubReg); - addRegReg(MIB, InRegLEA, true, InRegLEA2, true); + addRegReg(MIB, InRegLEA, true, X86::NoSubRegister, InRegLEA2, true, + X86::NoSubRegister); } if (LV && IsKill2 && InsMI2) LV->replaceKillInstruction(Src2, MI, *InsMI2); @@ -1589,6 +1591,7 @@ MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI, // the first call inserted a COPY from Src2 and marked it as killed. isKill = isKill2; SrcReg = SrcReg2; + SrcSubReg = SrcSubReg2; } else { if (!classifyLEAReg(MI, Src, Opc, /*AllowSP=*/true, SrcReg, SrcSubReg, isKill, ImplicitOp, LV, LIS)) @@ -1601,7 +1604,8 @@ MachineInstr *X86InstrInfo::convertToThreeAddress(MachineInstr &MI, if (ImplicitOp2.getReg() != 0) MIB.add(ImplicitOp2); - NewMI = addRegReg(MIB, SrcReg, isKill, SrcReg2, isKill2); // FIXME: Subregs + NewMI = + addRegReg(MIB, SrcReg, isKill, SrcSubReg, SrcReg2, isKill2, SrcSubReg2); // Add kills if classifyLEAReg created a new register. if (LV) { From d15163e85bb90f683d21e7a313078843807b6a01 Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Tue, 18 Feb 2025 23:13:46 +0700 Subject: [PATCH 3/3] fix --- llvm/lib/Target/X86/X86InstrInfo.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp b/llvm/lib/Target/X86/X86InstrInfo.cpp index 037fce3d3a344c..d756e73659a240 100644 --- a/llvm/lib/Target/X86/X86InstrInfo.cpp +++ b/llvm/lib/Target/X86/X86InstrInfo.cpp @@ -1172,6 +1172,8 @@ bool X86InstrInfo::classifyLEAReg(MachineInstr &MI, const MachineOperand &Src, unsigned SubReg = Src.getSubReg(); isKill = MI.killsRegister(SrcReg, /*TRI=*/nullptr); + NewSrcSubReg = X86::NoSubRegister; + // For both LEA64 and LEA32 the register already has essentially the right // type (32-bit or 64-bit) we may just need to forbid SP. if (Opc != X86::LEA64_32r) { @@ -1199,6 +1201,7 @@ bool X86InstrInfo::classifyLEAReg(MachineInstr &MI, const MachineOperand &Src, // Virtual register of the wrong class, we have to create a temporary 64-bit // vreg to feed into the LEA. NewSrc = MF.getRegInfo().createVirtualRegister(RC); + NewSrcSubReg = X86::NoSubRegister; MachineInstr *Copy = BuildMI(*MI.getParent(), MI, MI.getDebugLoc(), get(TargetOpcode::COPY)) .addReg(NewSrc, RegState::Define | RegState::Undef, X86::sub_32bit)