[MachineInstr] Move MIParser's DBG_VALUE RegState::Debug invariant into MachineInstr::addOperand

[jackoalan]

Summary

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.

[StephenTozer]

This looks like a strict improvement, since the new location for calling setIsDebug will catch all the old cases and more. There is one gap though - MachineOperand->ChangeToRegister(0, false) can be called on an operand for a DBG_VALUE (and is in multiple places), which sounds like it could cause the same problem you've observed; plugging this gap may require adding a similar block to that function.

This could probably do with a test - isDebug isn't printed to MIR, and it sounds like the issue only appears on a backend that isn't in LLVM, so a lit test probably isn't feasible. A simple unit test could work though: possibly a test in MachineInstrTest that creates a MachineInstr, adds MachineOperands, and confirms that they're debug registers.

[jackoalan]

Cover MachineOperand::ChangeToRegister with invariant. Add unit test to MachineInstrTest.cpp

[MatzeB]

Seems to me that the most important thing would be to add a check to MachineVerifier so we consistently catch problems. Adding automation to addOperand is fine, but there are enough ways to change instructions and operands with other APIs.

If you could find the time to add a check to MachineVerifier that would be fantastic! Though we can also leave that for another time, up to you.

Changes so far look fine, but if you add the automation, could you go through the sourcecode and remove manual usage of setIsDebug() then?

[jackoalan]

Shouldn't be too difficult to add the MachineVerifier check and remove the setIsDebug calls. Shall I also remove RegState::Debug in the context of DBG_VALUE builders?

[jackoalan]

Also curious if invariant debug register operands are applicable to other debug instructions (DBG_INSTR_REF, DBG_PHI, DBG_LABEL)

[jackoalan]

Add check to MachineVerifier. Remove explicit debug operand flag uses.

[StephenTozer]

Very nice cleanup!

Also curious if invariant debug register operands are applicable to other debug instructions (DBG_INSTR_REF, DBG_PHI, DBG_LABEL)

I think that this change should probably include DBG_PHIs as well - you could probably just replace isDebugValue() with isDebugInstr(), as any current or future debug instructions that use registers should have them set isDebug.

[jackoalan]

Apply to all debug instructions according to MachineInstr::isDebugInstr

[MatzeB]

Thanks, great work cleaning all up all the use sites! I have some comments on the verifier changes, then this will be good to go.

llvm/lib/CodeGen/MachineVerifier.cpp

1894–1895 ↗

(On Diff #377360)

• Use isDebugInstr() here as well and tweak message.
• Also check the reverse: non-debug instructions must not use the debug flag.
• Also test for isUse() for good measure (even though it should not be possible for debug instruction to ever define regs).

1963–1966 ↗

(On Diff #377360)

It seems a similar check already existed but was incorrectly only put into the isPhysicalRegister() branch... We can drop it now I guess.

[jackoalan]

Remove now-redundant MachineVerifier check. Also check debug instructions on uses only and non-debug instruction case.

[MatzeB]

LGTM

[jackoalan]

Thank you for the feedback! Would you mind committing this? --author="Jack Andersen <[e-mail]>"

llvm/lib/CodeGen/MIRParser/MIParser.cpp

while (!Token.isNewlineOrEOF() && Token.isNot(MIToken::kw_pre_instr_symbol) &&
          Token.isNot(MIToken::kw_heap_alloc_marker) &&
          Token.isNot(MIToken::kw_debug_location) &&
          Token.isNot(MIToken::kw_debug_instr_number) &&
          Token.isNot(MIToken::coloncolon) && Token.isNot(MIToken::lbrace)) {
     auto Loc = Token.location();
     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) ||
         Token.is(MIToken::lbrace))
       break;
     if (Token.isNot(MIToken::comma))
       return error("expected ',' before the next machine operand");
     lex();

llvm/lib/CodeGen/MachineInstr.cpp

if (!isImpReg) {
         int DefIdx = MCID->getOperandConstraint(OpNo, MCOI::TIED_TO);
         if (DefIdx != -1)
           tieOperands(DefIdx, OpNo);
       }
       // If the register operand is flagged as early, mark the operand as such.
       if (MCID->getOperandConstraint(OpNo, MCOI::EARLY_CLOBBER) != -1)
         NewMO->setIsEarlyClobber(true);
     }
+    // Ensure DBG_VALUE* sets RegState::Debug on all register use operands.
+    if (NewMO->isUse() && isDebugValue())
+      NewMO->setIsDebug();
   }
 }
 
 /// RemoveOperand - Erase an operand  from an instruction, leaving it with one
 /// fewer operand than it started with.
 ///
 void MachineInstr::RemoveOperand(unsigned OpNo) {
   assert(OpNo < getNumOperands() && "Invalid operand number");

llvm/lib/CodeGen/MachineOperand.cpp

void MachineOperand::ChangeToRegister(Register Reg, bool isDef, bool isImp,
   if (MachineFunction *MF = getMFIfAvailable(*this))
     RegInfo = &MF->getRegInfo();
   // If this operand is already a register operand, remove it from the
   // register's use/def lists.
   bool WasReg = isReg();
   if (RegInfo && WasReg)
     RegInfo->removeRegOperandFromUseList(this);
 
+  // Ensure DBG_VALUE* sets RegState::Debug on all register use operands.
+  const MachineInstr *MI = getParent();
+  if (!isDef && MI && MI->isDebugValue())
+    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");
   OpKind = MO_Register;
   SmallContents.RegNo = Reg;
   SubReg_TargetFlags = 0;
   IsDef = isDef;
   IsImp = isImp;

llvm/unittests/CodeGen/MachineInstrTest.cpp

TEST(MachineInstrExtraInfo, RemoveExtraInfo) {
 
   MI->setMemRefs(*MF, {});
   ASSERT_TRUE(MI->memoperands_empty());
   ASSERT_FALSE(MI->getPreInstrSymbol());
   ASSERT_FALSE(MI->getPostInstrSymbol());
   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}) {
+    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");
 
 } // end namespace