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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked 6 inline comments as done.
jmorse added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:240
       uint64_t RegNo;
       SpillLoc SpillLocation;
       int64_t Immediate;
----------------
djtodoro wrote:
> aprantl wrote:
> > aprantl wrote:
> > > It looks like this is exploding the size of the union. Could SpillLocation be made to fit into 64 bits, or does the extra 8 bytes not matter, or should it be an index into a side-table in the union?
> > At some point we'll cross over the point were this should be inheritance instead of a union.
> +1
> 
> I think we should think about refactoring the `VarLoc`, by creating several structures representing various locations. That should improve readability.
I think 32 bits is needed for both the offset and slot size, as it's feasible (if unlikely) that objects more than 2^16 could appear on the stack. However, there's probably no point storing the frame register in the spill location as it doesn't vary. The latest revision removes storing the base register, and assumes we use the frame register everywhere. This lets us move back to just having a single 8-byte union.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1546
+      unsigned PrependFlags = DIExpression::ApplyOffset;
+      DIExpr = DIExpression::prepend(DIExpr, PrependFlags, Offset);
+      MI.getOperand(3).setMetadata(DIExpr);
----------------
djtodoro wrote:
> we do not need the `PrependFlags`, so:
> 
>  `DIExpression::prepend(DIExpr, DIExpression::ApplyOffset, Offset);`
Done, cheers!


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

https://reviews.llvm.org/D69178