[PATCH] D123599: [DebugInfo][InstrRef] Describe value sizes when spilt to stack

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked an inline comment as done.
jmorse added a comment.

(Addressing inline comments -- note that they've now shifted down a few lines).



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:897
+        MIB.addImm(0);
+      } else if (VariableSize && *VariableSize != ValueSize) {
+        // We're loading a value off the stack that's not the same size as the
----------------
Orlando wrote:
> if `ValueSize > VariableSize` then presumably a standard `DW_OP_deref` would do the job? (this distinction may only save a few bytes, so may not be worth the additional case, it was just a thought).
I think in that case we get into questions about endian-ness -- it's alright on little endian I think, but not large. Rather than trying to think about that, IMO it's better to be explicit.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:904
+        Expr = DIExpression::prependOpcodes(Expr, Ops, true);
+        unsigned Flags = DIExpression::StackValue | DIExpression::ApplyOffset;
+        Expr = TRI.prependOffsetExpression(Expr, Flags, Spill.SpillOffset);
----------------
Orlando wrote:
> nit: Any reason to use intermediate variable for the flags here but when it's not done in the other branches?
Uuuhhhh. No, purely a line wrapping consideration. I can pick one and stick to it if you've got an opinion on which way it should be.


================
Comment at: llvm/test/DebugInfo/MIR/X86/livedebugvalues_load_in_loop.mir:55
   !5 = distinct !DIGlobalVariable(name: "start", scope: !0, file: !1, line: 4, type: !6, isLocal: false, isDefinition: true)
-  !6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !6 = !DIBasicType(name: "int", size: 64, encoding: DW_ATE_signed)
   !7 = !{i32 2, !"Dwarf Version", i32 4}
----------------
Orlando wrote:
> What is this change for?
I usually copy-and-paste metadata from other tests to avoid trimming it all over again, and then join it up with the IR / MIR that I want to test. Usually the size of the variable doesn't matter -- however 


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

https://reviews.llvm.org/D123599