[PATCH] D150216: Add support for salvaging debug info from icmp instrcuctions.

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

A few comments and questions inline - glad to see more operators being added to the salvageable list!



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2049-2051
+template <typename IntType>
+Value *getSalvageOpsForIcmpOp(ICmpInst *Icmp, IntType CurrentLocOps,
+                              SmallVectorImpl<IntType> &Opcodes,
----------------
Is there a reason for the `IntType` template here? At the moment we wouldn't expect any type other than `uint64_t` to appear here, as it's the width of a DWARF expression operand.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2055-2057
+  auto *ConstInt = dyn_cast<ConstantInt>(Icmp->getOperand(1));
+  if (!ConstInt)
+    return nullptr;
----------------
I don't think this early-exit to prevent a large DIExpression should be necessary - the only functions that call into `salvageDebugInfoImpl` either don't accept any additional values, or have a limit on the size of the DIExpression they will produce.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2062-2066
+  if (!CurrentLocOps) {
+    Opcodes.append({dwarf::DW_OP_LLVM_arg, 0});
+    CurrentLocOps = 1;
+  }
+  Opcodes.append({dwarf::DW_OP_LLVM_arg, CurrentLocOps});
----------------
I think this is the cause of the (as far as I can tell) erroneous DW_OP_LLVM_args in the expression in the test; these steps should only happen if you're salvaging a variadic expression, i.e. if `Icmp->getOperand(1)` is an `Instruction` rather than a `ConstantInt` - it should be fine if you just delete these lines outright, unless you change the function to also support salvaging a pair of `Instruction`s.

To summarize the intention of this code, `DW_OP_LLVM_arg` should only appear in variadic expressions as a way to represent the list of SSA values being passed to the expression. The first four lines check whether the expression is already variadic and converts it to be variadic if not, and then the last line adds the `LLVM_arg` for the new SSA value (which in this case does not exist), while the SSA value is added to `AdditionalValues`. The existing functions are not commented or documented very well, my apologies!


================
Comment at: llvm/test/DebugInfo/salvage-icmp.ll:6-8
+; CHECK: call void @llvm.dbg.value(metadata !DIArgList(i32 %a),
+; CHECK-SAME: ![[VAR_C:[0-9]+]],
+; CHECK-SAME: !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 10, DW_OP_constu, 0, DW_OP_ne, DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_arg, 9, DW_OP_constu, 0, DW_OP_eq, DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_arg, 8, DW_OP_constu, 1, DW_OP_gt, DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_arg, 7, DW_OP_consts, 18446744073709551615, DW_OP_gt, DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_arg, 6, DW_OP_constu, 2, DW_OP_ge, DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_arg, 5, DW_OP_consts, 18446744073709551614, DW_OP_ge, DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_arg, 4, DW_OP_constu, 3, DW_OP_lt, DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_arg, 3, DW_OP_consts, 18446744073709551613, DW_OP_lt, DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_arg, 2, DW_OP_constu, 4, DW_OP_le, DW_OP_LLVM_convert, 1, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_LLVM_arg, 1, DW_OP_consts, 18446744073709551612, DW_OP_le, DW_OP_stack_value))
----------------
Does this test pass as written? I see references to various values of `DW_OP_LLVM_arg` in the expression, but the `!DIArgList` contains 1 element - I'd expect this to trigger an assertion.


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

https://reviews.llvm.org/D150216