[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:6247
+  } else
+    DVI.setExpression(DIExpression::get(DVI.getContext(), Ops));
+}
----------------
StephenTozer wrote:
> 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.
> 
> 
Ok, I had failed to make my intentions clear with these functions. But the method wa messy anyway, I've tried to simplify things by adding the function numLLVMArgOps that determines the number of DW_OP_LLVM_ARG ops present in an expression. The count is then used to determine if a DIArglist should be used and if the expression can be shortened. I am still not completely happy with the implementation but i think it is an improvement.


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

https://reviews.llvm.org/D120169