From bd4dad87f421db82430f9958b52fbccc69d91b16 Mon Sep 17 00:00:00 2001 From: Jack Andersen Date: Thu, 7 Oct 2021 16:02:30 +0100 Subject: [PATCH] [MachineInstr] Move MIParser's DBG_VALUE RegState::Debug invariant into MachineInstr::addOperand Based on the reasoning of D53903, register operands of DBG_VALUE are invariably treated as RegState::Debug operands. This change enforces this invariant as part of MachineInstr::addOperand so that all passes emit this flag consistently. RegState::Debug is inconsistently set on DBG_VALUE registers throughout LLVM. This runs the risk of a filtering iterator like MachineRegisterInfo::reg_nodbg_iterator to process these operands erroneously when not parsed from MIR sources. This issue was observed in the development of the llvm-mos fork which adds a backend that relies on physical register operands much more than existing targets. Physical RegUnit 0 has the same numeric encoding as $noreg (indicating an undef for DBG_VALUE). Allowing debug operands into the machine scheduler correlates $noreg with RegUnit 0 (i.e. a collision of register numbers with different zero semantics). Eventually, this causes an assert where DBG_VALUE instructions are prohibited from participating in live register ranges. Reviewed By: MatzeB, StephenTozer Differential Revision: https://reviews.llvm.org/D110105 --- .../LiveDebugValues/InstrRefBasedImpl.cpp | 11 ++++---- .../LiveDebugValues/VarLocBasedImpl.cpp | 2 -- llvm/lib/CodeGen/MIRParser/MIParser.cpp | 4 --- llvm/lib/CodeGen/MachineFunction.cpp | 3 +-- llvm/lib/CodeGen/MachineInstr.cpp | 11 +++++--- llvm/lib/CodeGen/MachineOperand.cpp | 5 ++++ llvm/lib/CodeGen/MachineVerifier.cpp | 13 +++++++--- llvm/lib/CodeGen/PrologEpilogInserter.cpp | 1 - .../lib/CodeGen/SelectionDAG/InstrEmitter.cpp | 6 ++--- .../SelectionDAG/SelectionDAGBuilder.cpp | 2 +- llvm/lib/Target/AMDGPU/SIFrameLowering.cpp | 1 - llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp | 1 - .../Target/NVPTX/NVPTXPrologEpilogPass.cpp | 1 - .../WebAssemblyReplacePhysRegs.cpp | 2 -- llvm/lib/Target/X86/X86FastISel.cpp | 2 -- .../CodeGen/GlobalISel/PatternMatchTest.cpp | 4 +-- llvm/unittests/CodeGen/MachineInstrTest.cpp | 26 +++++++++++++++++++ 17 files changed, 59 insertions(+), 36 deletions(-) diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp index cc30bfc721f1..51d47f8359e4 100644 --- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp +++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp @@ -764,8 +764,8 @@ public: const DIExpression *Expr = Properties.DIExpr; if (!MLoc) { // No location -> DBG_VALUE $noreg - MIB.addReg(0, RegState::Debug); - MIB.addReg(0, RegState::Debug); + MIB.addReg(0); + MIB.addReg(0); } else if (LocIdxToLocID[*MLoc] >= NumRegs) { unsigned LocID = LocIdxToLocID[*MLoc]; const SpillLoc &Spill = SpillLocs[LocID - NumRegs + 1]; @@ -774,15 +774,15 @@ public: Expr = TRI->prependOffsetExpression(Expr, DIExpression::ApplyOffset, Spill.SpillOffset); unsigned Base = Spill.SpillBase; - MIB.addReg(Base, RegState::Debug); + MIB.addReg(Base); MIB.addImm(0); } else { unsigned LocID = LocIdxToLocID[*MLoc]; - MIB.addReg(LocID, RegState::Debug); + MIB.addReg(LocID); if (Properties.Indirect) MIB.addImm(0); else - MIB.addReg(0, RegState::Debug); + MIB.addReg(0); } MIB.addMetadata(Var.getVariable()); @@ -1220,7 +1220,6 @@ public: DIExpression::prepend(Prop.DIExpr, DIExpression::EntryValue); Register Reg = MTracker->LocIdxToLocID[Num.getLoc()]; MachineOperand MO = MachineOperand::CreateReg(Reg, false); - MO.setIsDebug(true); PendingDbgValues.push_back(emitMOLoc(MO, Var, {NewExpr, Prop.Indirect})); return true; diff --git a/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp index a026e57668d4..43fc17ac10c8 100644 --- a/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp +++ b/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp @@ -546,7 +546,6 @@ private: EVKind == EntryValueLocKind::EntryValueKind ? Orig.getReg() : Register(Loc.RegNo), false)); - MOs.back().setIsDebug(); break; case MachineLocKind::SpillLocKind: { // Spills are indirect DBG_VALUEs, with a base register and offset. @@ -568,7 +567,6 @@ private: DIExpr = DIExpression::appendOpsToArg(DIExpr, Ops, I); } MOs.push_back(MachineOperand::CreateReg(Base, false)); - MOs.back().setIsDebug(); break; } case MachineLocKind::ImmediateKind: { diff --git a/llvm/lib/CodeGen/MIRParser/MIParser.cpp b/llvm/lib/CodeGen/MIRParser/MIParser.cpp index 3dc91b3b87e9..d40c85742ec0 100644 --- a/llvm/lib/CodeGen/MIRParser/MIParser.cpp +++ b/llvm/lib/CodeGen/MIRParser/MIParser.cpp @@ -1011,10 +1011,6 @@ bool MIParser::parse(MachineInstr *&MI) { Optional TiedDefIdx; if (parseMachineOperandAndTargetFlags(OpCode, Operands.size(), MO, TiedDefIdx)) return true; - if ((OpCode == TargetOpcode::DBG_VALUE || - OpCode == TargetOpcode::DBG_VALUE_LIST) && - MO.isReg()) - MO.setIsDebug(); Operands.push_back( ParsedMachineOperand(MO, Loc, Token.location(), TiedDefIdx)); if (Token.isNewlineOrEOF() || Token.is(MIToken::coloncolon) || diff --git a/llvm/lib/CodeGen/MachineFunction.cpp b/llvm/lib/CodeGen/MachineFunction.cpp index bd13c280a82f..e118d6d20690 100644 --- a/llvm/lib/CodeGen/MachineFunction.cpp +++ b/llvm/lib/CodeGen/MachineFunction.cpp @@ -1158,7 +1158,7 @@ auto MachineFunction::salvageCopySSA(MachineInstr &MI) // Create DBG_PHI for specified physreg. auto Builder = BuildMI(InsertBB, InsertBB.getFirstNonPHI(), DebugLoc(), TII.get(TargetOpcode::DBG_PHI)); - Builder.addReg(State.first, RegState::Debug); + Builder.addReg(State.first); unsigned NewNum = getNewDebugInstrNum(); Builder.addImm(NewNum); return ApplySubregisters({NewNum, 0u}); @@ -1171,7 +1171,6 @@ void MachineFunction::finalizeDebugInstrRefs() { const MCInstrDesc &RefII = TII->get(TargetOpcode::DBG_VALUE); MI.setDesc(RefII); MI.getOperand(1).ChangeToRegister(0, false); - MI.getOperand(0).setIsDebug(); }; if (!useDebugInstrRef()) diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp index 0707945e7fb7..5c4f75e9ceb9 100644 --- a/llvm/lib/CodeGen/MachineInstr.cpp +++ b/llvm/lib/CodeGen/MachineInstr.cpp @@ -294,6 +294,9 @@ void MachineInstr::addOperand(MachineFunction &MF, const MachineOperand &Op) { if (MCID->getOperandConstraint(OpNo, MCOI::EARLY_CLOBBER) != -1) NewMO->setIsEarlyClobber(true); } + // Ensure debug instructions set debug flag on register uses. + if (NewMO->isUse() && isDebugInstr()) + NewMO->setIsDebug(); } } @@ -2111,11 +2114,11 @@ MachineInstrBuilder llvm::BuildMI(MachineFunction &MF, const DebugLoc &DL, assert(cast(Expr)->isValid() && "not an expression"); assert(cast(Variable)->isValidLocationForIntrinsic(DL) && "Expected inlined-at fields to agree"); - auto MIB = BuildMI(MF, DL, MCID).addReg(Reg, RegState::Debug); + auto MIB = BuildMI(MF, DL, MCID).addReg(Reg); if (IsIndirect) MIB.addImm(0U); else - MIB.addReg(0U, RegState::Debug); + MIB.addReg(0U); return MIB.addMetadata(Variable).addMetadata(Expr); } @@ -2134,7 +2137,7 @@ MachineInstrBuilder llvm::BuildMI(MachineFunction &MF, const DebugLoc &DL, if (IsIndirect) MIB.addImm(0U); else - MIB.addReg(0U, RegState::Debug); + MIB.addReg(0U); return MIB.addMetadata(Variable).addMetadata(Expr); } @@ -2153,7 +2156,7 @@ MachineInstrBuilder llvm::BuildMI(MachineFunction &MF, const DebugLoc &DL, MIB.addMetadata(Variable).addMetadata(Expr); for (const MachineOperand &MO : MOs) if (MO.isReg()) - MIB.addReg(MO.getReg(), RegState::Debug); + MIB.addReg(MO.getReg()); else MIB.add(MO); return MIB; diff --git a/llvm/lib/CodeGen/MachineOperand.cpp b/llvm/lib/CodeGen/MachineOperand.cpp index b8ba0453d24c..4d080e1a4f82 100644 --- a/llvm/lib/CodeGen/MachineOperand.cpp +++ b/llvm/lib/CodeGen/MachineOperand.cpp @@ -250,6 +250,11 @@ void MachineOperand::ChangeToRegister(Register Reg, bool isDef, bool isImp, if (RegInfo && WasReg) RegInfo->removeRegOperandFromUseList(this); + // Ensure debug instructions set debug flag on register uses. + const MachineInstr *MI = getParent(); + if (!isDef && MI && MI->isDebugInstr()) + isDebug = true; + // Change this to a register and set the reg#. assert(!(isDead && !isDef) && "Dead flag on non-def"); assert(!(isKill && isDef) && "Kill flag on def"); diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp index d0aead2d35bd..1c304345250f 100644 --- a/llvm/lib/CodeGen/MachineVerifier.cpp +++ b/llvm/lib/CodeGen/MachineVerifier.cpp @@ -1889,6 +1889,15 @@ MachineVerifier::visitMachineOperand(const MachineOperand *MO, unsigned MONum) { switch (MO->getType()) { case MachineOperand::MO_Register: { + // Verify debug flag on debug instructions. Check this first because reg0 + // indicates an undefined debug value. + if (MI->isDebugInstr() && MO->isUse()) { + if (!MO->isDebug()) + report("Register operand must be marked debug", MO, MONum); + } else if (MO->isDebug()) { + report("Register operand must not be marked debug", MO, MONum); + } + const Register Reg = MO->getReg(); if (!Reg) return; @@ -1955,10 +1964,6 @@ MachineVerifier::visitMachineOperand(const MachineOperand *MO, unsigned MONum) { return; } } - if (MI->isDebugValue() && MO->isUse() && !MO->isDebug()) { - report("Use-reg is not IsDebug in a DBG_VALUE", MO, MONum); - return; - } } else { // Virtual register. const TargetRegisterClass *RC = MRI->getRegClassOrNull(Reg); diff --git a/llvm/lib/CodeGen/PrologEpilogInserter.cpp b/llvm/lib/CodeGen/PrologEpilogInserter.cpp index b736aa213d09..9a4f70a6070f 100644 --- a/llvm/lib/CodeGen/PrologEpilogInserter.cpp +++ b/llvm/lib/CodeGen/PrologEpilogInserter.cpp @@ -1253,7 +1253,6 @@ void PEI::replaceFrameIndices(MachineBasicBlock *BB, MachineFunction &MF, StackOffset Offset = TFI->getFrameIndexReference(MF, FrameIdx, Reg); Op.ChangeToRegister(Reg, false /*isDef*/); - Op.setIsDebug(); const DIExpression *DIExpr = MI.getDebugExpression(); diff --git a/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp b/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp index 02cccdcaea22..c1bb65409282 100644 --- a/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp @@ -722,7 +722,7 @@ void InstrEmitter::AddDbgValueLocationOps( MIB.addFrameIndex(Op.getFrameIx()); break; case SDDbgOperand::VREG: - MIB.addReg(Op.getVReg(), RegState::Debug); + MIB.addReg(Op.getVReg()); break; case SDDbgOperand::SDNODE: { SDValue V = SDValue(Op.getSDNode(), Op.getResNo()); @@ -862,7 +862,7 @@ MachineInstr *InstrEmitter::EmitDbgNoLocation(SDDbgValue *SD) { DebugLoc DL = SD->getDebugLoc(); auto MIB = BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE)); MIB.addReg(0U); - MIB.addReg(0U, RegState::Debug); + MIB.addReg(0U); MIB.addMetadata(Var); MIB.addMetadata(Expr); return &*MIB; @@ -898,7 +898,7 @@ InstrEmitter::EmitDbgValueFromSingleOp(SDDbgValue *SD, if (SD->isIndirect()) MIB.addImm(0U); else - MIB.addReg(0U, RegState::Debug); + MIB.addReg(0U); return MIB.addMetadata(Var).addMetadata(Expr); } diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp index 43a10f523d66..0354e1d30b7a 100644 --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp @@ -5518,7 +5518,7 @@ bool SelectionDAGBuilder::EmitFuncArgumentDbgValue( // pointing at the VReg, which will be patched up later. auto &Inst = TII->get(TargetOpcode::DBG_INSTR_REF); auto MIB = BuildMI(MF, DL, Inst); - MIB.addReg(Reg, RegState::Debug); + MIB.addReg(Reg); MIB.addImm(0); MIB.addMetadata(Variable); auto *NewDIExpr = FragExpr; diff --git a/llvm/lib/Target/AMDGPU/SIFrameLowering.cpp b/llvm/lib/Target/AMDGPU/SIFrameLowering.cpp index a567e89ac3b7..3ec82fe48a24 100644 --- a/llvm/lib/Target/AMDGPU/SIFrameLowering.cpp +++ b/llvm/lib/Target/AMDGPU/SIFrameLowering.cpp @@ -1202,7 +1202,6 @@ void SIFrameLowering::processFunctionBeforeFrameFinalized( if (MI.isDebugValue() && MI.getOperand(0).isFI() && SpillFIs[MI.getOperand(0).getIndex()]) { MI.getOperand(0).ChangeToRegister(Register(), false /*isDef*/); - MI.getOperand(0).setIsDebug(); } } } diff --git a/llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp b/llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp index 38b9d85b653b..8080e09bc9d1 100644 --- a/llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp +++ b/llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp @@ -369,7 +369,6 @@ bool SILowerSGPRSpills::runOnMachineFunction(MachineFunction &MF) { if (MI.isDebugValue() && MI.getOperand(0).isFI() && SpillFIs[MI.getOperand(0).getIndex()]) { MI.getOperand(0).ChangeToRegister(Register(), false /*isDef*/); - MI.getOperand(0).setIsDebug(); } } } diff --git a/llvm/lib/Target/NVPTX/NVPTXPrologEpilogPass.cpp b/llvm/lib/Target/NVPTX/NVPTXPrologEpilogPass.cpp index 8e2299e65222..16fbe1a65562 100644 --- a/llvm/lib/Target/NVPTX/NVPTXPrologEpilogPass.cpp +++ b/llvm/lib/Target/NVPTX/NVPTXPrologEpilogPass.cpp @@ -74,7 +74,6 @@ bool NVPTXPrologEpilogPass::runOnMachineFunction(MachineFunction &MF) { auto Offset = TFI.getFrameIndexReference(MF, Op.getIndex(), Reg); Op.ChangeToRegister(Reg, /*isDef=*/false); - Op.setIsDebug(); const DIExpression *DIExpr = MI.getDebugExpression(); if (MI.isNonListDebugValue()) { DIExpr = TRI.prependOffsetExpression(MI.getDebugExpression(), DIExpression::ApplyOffset, Offset); diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp index dc854ba573c3..71f0bd28e1be 100644 --- a/llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp +++ b/llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp @@ -101,8 +101,6 @@ bool WebAssemblyReplacePhysRegs::runOnMachineFunction(MachineFunction &MF) { } } MO.setReg(VReg); - if (MO.getParent()->isDebugValue()) - MO.setIsDebug(); Changed = true; } } diff --git a/llvm/lib/Target/X86/X86FastISel.cpp b/llvm/lib/Target/X86/X86FastISel.cpp index da99e178c057..d5e7e2f10820 100644 --- a/llvm/lib/Target/X86/X86FastISel.cpp +++ b/llvm/lib/Target/X86/X86FastISel.cpp @@ -2784,8 +2784,6 @@ bool X86FastISel::fastLowerIntrinsicCall(const IntrinsicInst *II) { if (!X86SelectAddress(DI->getAddress(), AM)) return false; const MCInstrDesc &II = TII.get(TargetOpcode::DBG_VALUE); - // FIXME may need to add RegState::Debug to any registers produced, - // although ESP/EBP should be the only ones at the moment. assert(DI->getVariable()->isValidLocationForIntrinsic(DbgLoc) && "Expected inlined-at fields to agree"); addFullAddress(BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, II), AM) diff --git a/llvm/unittests/CodeGen/GlobalISel/PatternMatchTest.cpp b/llvm/unittests/CodeGen/GlobalISel/PatternMatchTest.cpp index b5f4e2266b07..19d9cbe1d3ea 100644 --- a/llvm/unittests/CodeGen/GlobalISel/PatternMatchTest.cpp +++ b/llvm/unittests/CodeGen/GlobalISel/PatternMatchTest.cpp @@ -499,8 +499,8 @@ TEST_F(AArch64GISelMITest, MatchMiscellaneous) { EXPECT_TRUE(mi_match(Reg, *MRI, m_OneNonDBGUse(m_GAdd(m_Reg(), m_Reg())))); // Add multiple debug uses of Reg. - B.buildInstr(TargetOpcode::DBG_VALUE, {}, {Reg})->getOperand(0).setIsDebug(); - B.buildInstr(TargetOpcode::DBG_VALUE, {}, {Reg})->getOperand(0).setIsDebug(); + B.buildInstr(TargetOpcode::DBG_VALUE, {}, {Reg}); + B.buildInstr(TargetOpcode::DBG_VALUE, {}, {Reg}); EXPECT_FALSE(mi_match(Reg, *MRI, m_OneUse(m_GAdd(m_Reg(), m_Reg())))); EXPECT_TRUE(mi_match(Reg, *MRI, m_OneNonDBGUse(m_GAdd(m_Reg(), m_Reg())))); diff --git a/llvm/unittests/CodeGen/MachineInstrTest.cpp b/llvm/unittests/CodeGen/MachineInstrTest.cpp index 82be17bac57b..15e22fe7bb03 100644 --- a/llvm/unittests/CodeGen/MachineInstrTest.cpp +++ b/llvm/unittests/CodeGen/MachineInstrTest.cpp @@ -386,6 +386,32 @@ TEST(MachineInstrExtraInfo, RemoveExtraInfo) { ASSERT_FALSE(MI->getHeapAllocMarker()); } +TEST(MachineInstrDebugValue, AddDebugValueOperand) { + LLVMContext Ctx; + Module Mod("Module", Ctx); + auto MF = createMachineFunction(Ctx, Mod); + + for (const unsigned short Opcode : + {TargetOpcode::DBG_VALUE, TargetOpcode::DBG_VALUE_LIST, + TargetOpcode::DBG_INSTR_REF, TargetOpcode::DBG_PHI, + TargetOpcode::DBG_LABEL}) { + const MCInstrDesc MCID = { + Opcode, 0, 0, + 0, 0, (1ULL << MCID::Pseudo) | (1ULL << MCID::Variadic), + 0, nullptr, nullptr, + nullptr}; + + auto *MI = MF->CreateMachineInstr(MCID, DebugLoc()); + MI->addOperand(*MF, MachineOperand::CreateReg(0, /*isDef*/ false)); + + MI->addOperand(*MF, MachineOperand::CreateImm(0)); + MI->getOperand(1).ChangeToRegister(0, false); + + ASSERT_TRUE(MI->getOperand(0).isDebug()); + ASSERT_TRUE(MI->getOperand(1).isDebug()); + } +} + static_assert(std::is_trivially_copyable::value, "trivially copyable");