[PATCH] D81852: [DebugInfo] Update MachineInstr interface to better support variadic DBG_VALUE instructions

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:466
   const MachineOperand& getOperand(unsigned i) const {
     assert(i < getNumOperands() && "getOperand() out of range!");
     return Operands[i];
----------------
StephenTozer wrote:
> vsk wrote:
> > Is it possible to take this further and assert that getOperand() is not called on a debug instruction?
> `getOperand()` is still used (internally) by the debug-specific getters, although it could be replaced by `Operands[i]`. I'm not sure if reducing the interface so drastically would be ideal though; code that is agnostic about the type of instruction it operates on - such as replacing register usage - would break for debug values, and have to be rewritten to accomodate. That wouldn't necessarily be a bad thing though, since we might actually want different code in all those places.
Ah that's right, there is a fair amount of code that transparently updates debug instructions. I ran into this some time ago while trying to make the MachinerRegisterInfo use/def iterators skip debug instructions by default. It can be hard to tell whether a piece of code is intentionally updating debug instructions.


================
Comment at: llvm/test/CodeGen/MIR/Generic/dbg-value-missing-loc.mir:41
   bb.0.entry:
-    DBG_VALUE 1, 2, 3, 4
+    DBG_VALUE 0, $noreg, !12, !DIExpression()
+    $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, debug-location !14
----------------
StephenTozer wrote:
> vsk wrote:
> > What necessitates this change?
> Some of the previous code dealing specifically with debug values had some statements along the lines of `if (MI.isDebugValue() && MI.getOperand(2).isMetadata()) {...}`. As of this patch we're now using specific functions, i.e. `getDebugVariable()`, that assert that the operand is the right type and then cast to it. AFAIUI this shouldn't be a breaking change as we never produce DBG_VALUEs without the "right" operands, like this test had.
I see that specific check in LiveDebugVariables, but I'm not sure why it would apply here since we're just running the verifier.

It's fairly common for backend authors to write tests with incomplete/malformed DBG_VALUEs as it's vastly more convenient than wiring up actual debug info metadata (see e.g. X86/branchfolding-debug-invariant.mir). It'd be great if we could keep that feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81852