[PATCH] D129372: [DebugInfo][NFC?] Add new MachineOperand type and change DBG_INSTR_REF syntax

[Jeremy Morse via Phabricator via llvm-commits]

jmorse requested changes to this revision.
jmorse added a comment.
This revision now requires changes to proceed.

This all seems fine and well, I've added some nits about spellings and how things present to the developer,

This is missing test changes to account for the fact that all LL -> MIR variable locations will now be spelt differently, that should lead to a bunch of tests failing. Plus: a round-trip test for the syntax of the new operand would be good.

Seeing how this patch moves everything over to use the new operand form, how about adding a MachineVerifier check that DBG_INSTR_REF's have the correct format? That's going to catch any MIR tests where the old syntax is used, which would break in strange ways when fed into LiveDebugValues.



================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1266
+  bool isDebugValueLike() const { return isDebugValue() || isDebugRef(); }
+  bool isDebugLabel() const { return getOpcode() == TargetOpcode::DBG_LABEL; }
   bool isDebugPHI() const { return getOpcode() == TargetOpcode::DBG_PHI; }
----------------
Un-necessary move of the label -- quite a nit, but future code archeologists looking at git blame will appreciate it not having been moved.


================
Comment at: llvm/include/llvm/CodeGen/MachineOperand.h:67
     MO_MCSymbol,          ///< MCSymbol reference (for debug/eh info)
+    MO_DbgInstrRef, ///< Integer indices referring to an instruction+operand
     MO_CFIIndex,    ///< MCCFIInstruction index.
----------------
Any particular reason for this not being at the end? I don't think it makes a difference, but on the principle of least surprise (and in case someone out there is preserving these things) it'd be better to append to this list.


================
Comment at: llvm/include/llvm/CodeGen/MachineOperand.h:599-602
+  unsigned getOpIdx() const {
+    assert(isDbgInstrRef() && "Wrong MachineOperand accessor");
+    return Contents.InstrRef.OpIdx;
+  }
----------------
I fear that this method name is going to be misinterpreted by the reader as returning what the operands index is in the owning instruction. Obviously this is a bother to think about, but worth considering. Possibly prefixing Dbg before 'Instr' and 'Op' will be clearer?

Also, for consistency, IMO it should be "Index" instead of Idx, the following method says "Index".

Similarly with the setters.


================
Comment at: llvm/lib/CodeGen/InlineSpiller.cpp:1066
     if (MI.isDebugValue()) {
+      assert(!MI.isDebugRef() && "Debug Instr Ref should not use spilled reg");
       // Modify DBG_VALUE now that the value is in a spill slot.
----------------
This seems out of place in a "adding-the-machine-operand-type" patch, would it be better re-homed into one of the later ones? If there's no specific error mode in mind where this could fire, it's probably ok to fold in as just a "general improvement"


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1324
     return false;
-
   const DILocalVariable *Var = MI.getDebugVariable();
----------------
Unrelated whitespace changes undesirable


================
Comment at: llvm/lib/CodeGen/MIRParser/MILexer.cpp:282
+      .Case("machine-block-address-taken",
+            MIToken::kw_machine_block_address_taken)
       .Default(MIToken::Identifier);
----------------
clang-format fixing best done in a separate / independent patch. I believe so long as it's an obvious 80-column fix or clang-format problem, you can just push it up without review.


================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:2721-2722
+  assert(Token.is(MIToken::kw_dbg_instr_ref));
+  // TODO: Do we need to verify that the values passed are able to fit inside an
+  // unsigned?
+
----------------
If there's an obvious way to assert for that, then yes please.


================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:1192-1194
+    const MCInstrDesc &RefII = TII->get(TargetOpcode::DBG_VALUE_LIST);
     MI.setDesc(RefII);
+    MI.getDebugOperand(0).setReg(0);
----------------
AFAIUI this isn't necessary at this stage (and could lead to test breakage), is that right? If so, better to fold into a "making DBG_VALUE_LIST everywhere" (or whatever it gets called in the end) patch.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:571-574
+  // DBG_INSTR_REF should not contain any virtual registers.
+  if (MI.isDebugRef())
+    return;
+
----------------
Isn't it necessary for the variable information to go into SeenDbgVars, so that a DBG_VALUE can't re-order with a DBG_INSTR_REF


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:1255
              "Expected inlined-at fields to agree");
+      if (!UseInstrRefDebugInfo || !Op->isReg()) {
         // A dbg.declare describes the address of a source variable, so lower it
----------------
IMO easier to read by keeping the original spelling (`UseInstrRefDebugInfo && Op->isReg()`) and just swapping the order of blocks -- inverting the logic causes the reader to think something complicated is going on.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129372/new/

https://reviews.llvm.org/D129372