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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

Still a further suggestion on the most recent change - it's a minor point, but necessary I think to not produce invalid expressions. Other than that, LGTM.



================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6260
+/// Write the new expression and new location ops for the dbg.value. If possible
+/// reduce the szie of the dbg.value intrinsic by omitting DIArglist. This
+/// can be omitted if:
----------------



================
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.
----------------
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!


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

https://reviews.llvm.org/D120169