[DebugInfo][5/N] Recognise extra indirection in instruction-referencing for NRVO tests

[jmorse]

Summary

In some scenarios, usually involving NRVO, we can issue indirect DBG_VALUEs after SelectionDAG, even in instruction referencing mode (if the variable is an argument). If the corresponding argument value is spilt to the stack, then we have:

• Indirection because it's on the stack,
• Indirection because of the nature of the variable (i.e., it was created with indirection),

However InstrRefBasedLDV only emits one level of indirection. This patch adds the second, by adding an extra DW_OP_deref if necessary.

The two tests modified fail otherwise -- they feature some NRVO, and require two levels of indirection to be correct.

(An alternate solution would be to stop emitting DBG_VALUEs with indirection -- that'll probably work, but I think it should be part of a refactor. InstrRefBasedLDV is agnostic as to whether it's working on DBG_INSTR_REFs or DBG_VALUEs, I might have baked more assumptions into it).

[StephenTozer]

Would it make sense to have these tests run twice, once with experimental-debug-variable-locations being true and once with false? Otherwise it looks like they will no longer cover the normal DBG_VALUE case with this patch.

[jmorse]

In D114364#3154243, @StephenTozer wrote:

Would it make sense to have these tests run twice, once with experimental-debug-variable-locations being true and once with false? Otherwise it looks like they will no longer cover the normal DBG_VALUE case with this patch.

Sometimes yes, sometimes no, see discussion with Orlando here: https://reviews.llvm.org/D114362#3151143

IMO these tests would benefit little as they're really targetted at LiveDebugValues, and shouldn't really be .ll tests. On the other hand, given that there isn't actually a test difference between the two implementations, it won't hurt to add some extra RUN lines, WDYT?

[StephenTozer]

In D114364#3154256, @jmorse wrote:

IMO these tests would benefit little as they're really targetted at LiveDebugValues, and shouldn't really be .ll tests. On the other hand, given that there isn't actually a test difference between the two implementations, it won't hurt to add some extra RUN lines, WDYT?

If I understand right, these tests do cover behaviour in the VarLocBasedImpl that we want to maintain - without the extra run lines, someone could theoretically run through the VarLoc impl and delete the "add a deref when spilling already-indirect offsets" code, and that would no longer cause a test failure as of this patch. If I've understood incorrectly then the patch LGTM, otherwise I think it'd be good to have the extra run lines (and with those lines, LGTM).

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

if (!MLoc) {
     // the variable is.
     if (Offset == 0) {
       const SpillLoc &Spill = SpillLocs[SpillID.id()];
       Expr = TRI.prependOffsetExpression(Expr, DIExpression::ApplyOffset,
                                          Spill.SpillOffset);
       unsigned Base = Spill.SpillBase;
       MIB.addReg(Base);
       MIB.addImm(0);
+
+      // Being on the stack makes this location indirect; if it was _already_
+      // indirect though, we need to add extra indirection. See this test for
+      // a scenario where this happens:
+      //     llvm/test/DebugInfo/X86/spill-nontrivial-param.ll
+      if (Properties.Indirect) {
+        std::vector<uint64_t> Elts = {dwarf::DW_OP_deref};
+        Expr = DIExpression::append(Expr, Elts);
+      }
     } else {
       // This is a stack location with a weird subregister offset: emit an undef
       // DBG_VALUE instead.
       MIB.addReg(0);
       MIB.addReg(0);
     }
   } else {
     // Non-empty, non-stack slot, must be a plain register.

llvm/test/DebugInfo/X86/spill-indirect-nrvo.ll

 ; RUN: llc < %s -experimental-debug-variable-locations=false | FileCheck -check-prefixes=CHECK,OPT %s
 ; RUN: llc -O0 < %s -experimental-debug-variable-locations=false | FileCheck -check-prefixes=CHECK,OPTNONE %s
+; RUN: llc < %s -experimental-debug-variable-locations=true | FileCheck -check-prefixes=CHECK,OPT %s
+; RUN: llc -O0 < %s -experimental-debug-variable-locations=true | FileCheck -check-prefixes=CHECK,OPTNONE %s
 
 ; Make sure we insert DW_OP_deref when spilling indirect DBG_VALUE instructions.
 
 ; C++ source:
 ; #define FORCE_SPILL() \
 ;   __asm volatile("" : : : \
 ;                    "rax", "rbx", "rcx", "rdx", "rsi", "rdi", "rbp", "r8", \
 ;                    "r9", "r10", "r11", "r12", "r13", "r14", "r15")

llvm/test/DebugInfo/X86/spill-nontrivial-param.ll

 ; RUN: llc < %s -experimental-debug-variable-locations=false | FileCheck %s
+; RUN: llc < %s -experimental-debug-variable-locations=true | FileCheck %s
 
 ; Make sure we insert DW_OP_deref when spilling indirect DBG_VALUE instructions.
 ; In this example, 'nt' is passed by address because it is not trivially
 ; copyable. When we spill the physical argument register at the barrier, we need
 ; to insert DW_OP_deref.
 
 ; #define FORCE_SPILL() \
 ;   __asm volatile("" : : : \