[PATCH] D120169: [Debuginfo][LSR} Add support for salvaging variadic dbg.value intrinsics [2/2]

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6247
+  } else
+    DVI.setExpression(DIExpression::get(DVI.getContext(), Ops));
+}
----------------
This does a good job of recognizing when the location use is complex, but it doesn't correctly handle the case where `DW_OP_LLVM_arg, 0` appears later in the expression. The block for `if (HasSimpleLocationUse)` is correct, but the `else` still assumes that we just have a simple non-variadic expression. What we need is 3 different cases:

  # Single use of arg 0 at the front: `(DW_OP_LLVM_arg, 0, ...)` -> Drop the first two elements of the expression.
  # Non-variadic expression: `(DW_OP_plus_uconst, 5, DW_OP_stack_value)` -> Just set the unmodified expression.
  # Complex use of arg 0: `(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 0, ...)` -> Drop the value altogether (or fixup with DW_OP_dup and DW_OP_swap, not recommended for this patch).

Right now this code handles the first two, and treats 3 the same as 2, which would result in an expression that does not use an arglist but contains DW_OP_LLVM_arg, which is currently an error.




================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6249-6251
+  if (NumPreLSRElements == 0 && SalvageExpression->getNumElements() != 0) {
+    SalvageExpression =
+        DIExpression::append(SalvageExpression, {dwarf::DW_OP_stack_value});
----------------
chrisjackson wrote:
> StephenTozer wrote:
> > This check could be simplified and made more correct by instead using `DIExpr->isComplex()`, since that also checks for DW_OP_LLVM_fragment and DW_OP_LLVM_tag_offset (which don't force stack_value). Also, as in the previous patch, it'd be better to use DIExpression's prepend function that accounts for fragments than directly appending the stack_value.
> I don't think prepend() functions as desired here, as it will call prependOpCodes() with an empty Ops, which prevents the stack terminator being applied.
My bad, that's correct - carry on. Just as long as SalvageExpression is guaranteed to not have DW_OP_stack_value already this should be fine.


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

https://reviews.llvm.org/D120169