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

[Chris Jackson via Phabricator via llvm-commits]

chrisjackson added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6165-6179
+    uint64_t IndexOffset = DestLocations.size() - 1;
+    DestLocations.insert(DestLocations.end(), LocationOps.begin() + 1,
+                         LocationOps.end());
+
+    for (const auto &Op : expr_ops()) {
+      if (Op.getOp() != dwarf::DW_OP_LLVM_arg) {
+        Op.appendToVector(DestExpr);
----------------
StephenTozer wrote:
> Example impl for the above suggestion.
Thanks!


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6211-6215
+  if (Ops[0] == dwarf::DW_OP_LLVM_arg && Ops[1] == 0) {
+    llvm::SmallVector<uint64_t, 6> ShortenedOps(llvm::drop_begin(Ops, 2));
+    DVI.setExpression(DIExpression::get(DVI.getContext(), ShortenedOps));
+  } else
+    DVI.setExpression(DIExpression::get(DVI.getContext(), Ops));
----------------
StephenTozer wrote:
> There should probably be an assertion in this function that `DW_OP_LLVM_arg` appears either exactly once in the DIExpr as the first operator, or not at all. Actually, it is also possible that `DW_OP_LLVM_arg, 0` could legally appear multiple times in the expression, which would be a little awkward to deal with. For example, if we salvaged the square of the IV, the expression might be `DIExpr(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 0, DW_OP_mul)`.
> 
> I think the suitable answer for this patch would just be to drop such expressions for now, so that it's not a blocker. As a slight tangent though, there is a general solution possible; for an expression `DIExpr([[A]], DW_OP_LLVM_arg, 0, [[B]])`, it is possible to transform it to a non-variadic form like so: `DIExpr(DW_OP_dup, [[A]], DW_OP_swap, [[B]])`. Personally I'd be happy with this not being added in this patch though, it's a bit much imo.
I've added a lambda to iterate over the expression and check fo multiple DW_OP_LLVM_arg uses.


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


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

https://reviews.llvm.org/D120169