[PATCH] D156545: [DebugInfo][InstrRef] Don't produce over-sized DW_OP_deref_size expressions for very wide stack spills

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

Orlando added a comment.
Herald added a subscriber: ormris.

Just a couple of inline nits/questions from me.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1163
+  // size of value and the size of variable portion being read, on big endian
+  // systems. Consider:
+  //
----------------
Does a test for this part already exist? I assume so, since this patch is just narrowing the use cases rather than adding the functionally, but wanted to check to be sure.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1175
+  unsigned ValueSizeInBits = getLocSizeInBits(MLoc);
+  unsigned DerefSizeInBytes = ValueSizeInBits / 8;
+  if (DerefSizeInBytes > PointerSizeBytes) {
----------------
Can `ValueSizeInBits` ever be a value that doesn't divide cleanly by 8? e.g. 1 from an `i1` type (I haven't looked at how `getLocSizeInBits` works). 

If so then I think we might need to use `divideCeil` from `MathExtras.h` here. Otherwise, with the code as is, a `1` would result in `DerefSizeInBytes` of `0`.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1181
+  } else if (auto Fragment = Var.getFragment()) {
+    // XXX why the non-complex bit.
+    unsigned VariableSizeInBits = Fragment->SizeInBits;
----------------
left over dev comment?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1300
+          unsigned ValueSizeInBits = getLocSizeInBits(MLoc);
+          unsigned DerefSizeInBytes = ValueSizeInBits / 8;
           OffsetOps.push_back(dwarf::DW_OP_deref_size);
----------------
Same question about bit sizes and divisions here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156545