[llvm] bd4dad8 - [MachineInstr] Move MIParser's DBG_VALUE RegState::Debug invariant into MachineInstr::addOperand
Stephen Tozer via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 7 08:09:33 PDT 2021
Author: Jack Andersen
Date: 2021-10-07T16:08:52+01:00
New Revision: bd4dad87f421db82430f9958b52fbccc69d91b16
URL: https://github.com/llvm/llvm-project/commit/bd4dad87f421db82430f9958b52fbccc69d91b16
DIFF: https://github.com/llvm/llvm-project/commit/bd4dad87f421db82430f9958b52fbccc69d91b16.diff
LOG: [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
Added:
Modified:
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp
llvm/lib/CodeGen/MIRParser/MIParser.cpp
llvm/lib/CodeGen/MachineFunction.cpp
llvm/lib/CodeGen/MachineInstr.cpp
llvm/lib/CodeGen/MachineOperand.cpp
llvm/lib/CodeGen/MachineVerifier.cpp
llvm/lib/CodeGen/PrologEpilogInserter.cpp
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp
llvm/lib/Target/NVPTX/NVPTXPrologEpilogPass.cpp
llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
llvm/lib/Target/X86/X86FastISel.cpp
llvm/unittests/CodeGen/GlobalISel/PatternMatchTest.cpp
llvm/unittests/CodeGen/MachineInstrTest.cpp
Removed:
################################################################################
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index cc30bfc721f1a..51d47f8359e44 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
@@ -764,8 +764,8 @@ class MLocTracker {
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 @@ class MLocTracker {
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 @@ class TransferTracker {
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 a026e57668d45..43fc17ac10c89 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp
@@ -546,7 +546,6 @@ class VarLocBasedLDV : public LDVImpl {
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 @@ class VarLocBasedLDV : public LDVImpl {
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 3dc91b3b87e97..d40c85742ec01 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<unsigned> 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 bd13c280a82f6..e118d6d206905 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 0707945e7fb76..5c4f75e9ceb98 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<DIExpression>(Expr)->isValid() && "not an expression");
assert(cast<DILocalVariable>(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 b8ba0453d24c7..4d080e1a4f824 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 d0aead2d35bd5..1c304345250f8 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 b736aa213d092..9a4f70a6070f8 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 02cccdcaea221..c1bb65409282e 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 43a10f523d660..0354e1d30b7a4 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 a567e89ac3b7e..3ec82fe48a240 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 38b9d85b653bd..8080e09bc9d1b 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 8e2299e652221..16fbe1a65562b 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 dc854ba573c36..71f0bd28e1beb 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 da99e178c0571..d5e7e2f10820d 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 b5f4e2266b07b..19d9cbe1d3ea3 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 82be17bac57b3..15e22fe7bb03d 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<MCOperand>::value,
"trivially copyable");
More information about the llvm-commits
mailing list