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

[Shubham Sandeep Rastogi via Phabricator via llvm-commits]

rastogishubham added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2049-2051
+template <typename IntType>
+Value *getSalvageOpsForIcmpOp(ICmpInst *Icmp, IntType CurrentLocOps,
+                              SmallVectorImpl<IntType> &Opcodes,
----------------
StephenTozer wrote:
> 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.
I am sorry about that, I was working on a patch where I was going to pass an `int64_t` for signed comparisons, I ended up realizing that a DW_OP_consts can be used with a `uint64_t` just fine, so I removed the template everywhere, but I guess I missed a place


================
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});
----------------
StephenTozer wrote:
> 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!
Thanks for explaining the logic to me, I did get a bit confused there


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

https://reviews.llvm.org/D150216