[PATCH] D69178: [DebugInfo] Use DBG_VALUEs IsIndirect field for describing stack spills

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a subscriber: inglorion.
vsk added a comment.

The high level approach makes sense to me. Restricting the meaning of DBG_VALUE's "IsIndirect" operand should reduce the complexity surrounding creating/updating debug instructions. And the ability to recognize loads-from-spills and propagate more DBG_VALUEs is a big plus.

+ @inglorion for any comment on test/CodeGen/MIR/X86/diexpr-win32.mir.



================
Comment at: lib/CodeGen/LiveDebugValues.cpp:223
+        uint64_t Hash1;
+        uint64_t Hash2;
+      } u;
----------------
aprantl wrote:
> This warrants some comment as to what those two hashes are used for.
I can't really tell right away what it's for, but suspect it's used in place of memcmp to do equality checking. If so, it may be simpler to just use memcmp.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:330
+        if (DIExpr->isImplicit()) {
+          SmallVector<uint64_t, 2> Ops = {dwarf::DW_OP_deref_size, Loc.SpillLocation.SpillSize};
+          DIExpr = DIExpression::prependOpcodes(DIExpr, Ops);
----------------
It looks like the diff needs to be clang-formatted.


================
Comment at: test/DebugInfo/X86/dbg-value-of-spill-with-fp.ll:17
+; CHECK-NEXT: DW_OP_breg4 ESP+{{[0-9]}}
+; CHECK-NEXT: DW_OP_breg4 ESP+{{[0-9]}}
+; CHECK-NEXT: DW_OP_reg0 EAX)
----------------
I'm assuming these two stack slots for %somearg are from before/after the first asm sideeffect. I suppose this is the repeated location spec you referred to? If so, wdyt of tightening this to:
```
DW_OP_breg4 ESP+[[SLOT:[0-9]]]
DW_OP_breg4 ESP+[[SLOT]]
```


================
Comment at: test/DebugInfo/X86/op_deref.ll:61
+; The restore of vla here causes the stack location to be clobbered going
+; back around the loop.
   %arrayidx = getelementptr inbounds i32, i32* %vla, i64 %idxprom, !dbg !23
----------------
Mind moving the XFAIL to the first line of the test, with a pointer to the more detailed comment within `for.body`, and one to the check line that fails?

Alternatively, if at least one location for vla survives, why not disable the failing check line(s) instead of xfailing the whole test? You might consider filing a PR about possible improvements related to this test.




Repository:
  rL LLVM

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

https://reviews.llvm.org/D69178