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

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

In D123599#3445402 <https://reviews.llvm.org/D123599#3445402>, @jmorse wrote:

> /me squints -- looks like the test doesn't cover the case of expressions with fragments, which I'll revise in later. (The patch has a case for that).

Pending the promised additional test, this LGTM with a couple of nits.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:865
+    }
+    unsigned ValueSize = getLocSizeInBits(*MLoc);
+
----------------
nit: `ValueSizeInBits` instead?


================
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
----------------
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).


================
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);
----------------
nit: Any reason to use intermediate variable for the flags here but when it's not done in the other branches?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:907
+        MIB.addReg(0);
+    } else if (Expr->isComplex()) {
+        // A variable with no size ambiguity, but with extra elements in it's
----------------
nit: Looks like the `else if` line is indented one level too few.


================
Comment at: llvm/test/DebugInfo/MIR/InstrRef/deref-spills-with-size.mir:169
+
+    ; Repeat again -- this time with no contents in the DIExpression. This test
+    ; that when we switch between a plain register location and a DWARF location
----------------



================
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}
----------------
What is this change for?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123599