[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:6271-6284
+  } else if (NumLLVMArgs > 1) {
+    // Multiple DW_OP_llvm_arg, so DIArgList is stricly necessary.
+    updateDVIWithLocations(*DVIRec.DVI, NewLocationOps, NewExpr);
+  } else if (NumLLVMArgs == 1) {
+    // There is only a single DW_OP_llvm_arg in the expression. If this
+    // is the first operand in the expression, it can be omitted along with
+    // DIArglist.
----------------
StephenTozer wrote:
> This is mostly there, but the `if` in the `NumLLVMArgs == 1` block looks off to me, as it implies that the single `DW_OP_LLVM_arg` might not be the first element in `NewExpr`. Rather than an `if`, it should be an `assert` - I don't believe we ever produce an expression that uses `DW_OP_LLVM_arg` in the middle of an expression and not at the start. Alternatively, if we want to accept such expressions (which I think is quite reasonable), then they should become `DBG_VALUE_LIST`s rather than `DBG_VALUE`s. Also, if there is only one LLVM_arg in the expression then its value should definitely be 0 - anything else would be a malformed expression (we assert elsewhere that we never skip an arg index, i.e. we cannot have an expr with args [0, 1, 3] - it must be [0, 1, 2] or [0, 1, 2, 3]).
> 
> Suggested implementation included!
I like this change a lot. I find your implementation clearer (and correct unlike mine!). I was not certain of the rules for DW_OP_LLVM_arg placement and this also makes it very clear. Thanks


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

https://reviews.llvm.org/D120169