[PATCH] D68945: [DebugInfo] Don't translate dbg.addr and similar intrinsics into indirect DBG_VALUEs

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked 2 inline comments as done.
jmorse added a comment.

In D68945#1708182 <https://reviews.llvm.org/D68945#1708182>, @aprantl wrote:

> Thanks a lot! When changing something like this, you might want to diff the debug info of a test program before and after the change and look for any unexpected changes not covered by our tests.


It looks like clang-3.4's X86ISelLowering.cpp produces identical debuginfo; I was expecting to see more PR41992 differences,

> Yes, a DW_OP_deref is preferable to the indirect bit. How hard would it be to convert the remaining use-cases to retire the flag?

Moderately complicated, there's currently a good use-case for IsIndirect, it lets the PrologEpilog pass distinguish:

- DBG_VALUEs that refer to the contents of a spill stack slot (IsIndirect=True)
- DBG_VALUEs that should evaluate to a stack pointer, not its value (IsIndirect=False)

There are other ways of implementing this though, such as examining the stack-slot type or LiveDebugVariables prepending a DW_OP_deref for slot-contents locations.

I'd like to keep the field around, but use it to pass spill-slot frame indexes to LiveDebugValues, more on that in another patch though.

Reid wrote:

> One concern that I have about folding everything into the DIExpression is that they are effectively leaked. [...] As a future alternative, maybe we could use the same representation (sequence of DWARF opcodes), but store them as MachineInstr operands.

That too might be achievable by putting the spill-slot frame index in DBG_VALUE insts.



================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1381
       // help debugging.  Probably the optimizer should not do this.
-      MIRBuilder.buildIndirectDbgValue(0, DI.getVariable(), DI.getExpression());
+      MIRBuilder.buildDirectDbgValue(0, DI.getVariable(), DI.getExpression());
     } else if (const auto *CI = dyn_cast<Constant>(V)) {
----------------
aprantl wrote:
> Out of curiosity: is passing `0` here really the same as creating an `undef`?
I think so -- both Indirect and Direct builders use the register-form of BuildMI and create DBG_VALUEs with $noreg as the first operand.

This change isn't technically necessary, but we may as well skip adding a spurious deref to a $noreg DBG_VALUE if we can.


================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:129
       .addFrameIndex(FI)
       .addImm(0)
       .addMetadata(Variable)
----------------
Note to self, I missed this, and it risks asserting later


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68945