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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This sounds like a good direction to take; a comment inline about whether the patch as-is only salvages icmps with one constant operand (it appears to salvage more?).

It might be worth relaxing that condition and allowing all icmps to be salvaged, if it doesn't significantly increase file size on some large code samples.



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1985
 
+void handleSSAValueOperands(uint64_t CurrentLocOps,
+                            SmallVectorImpl<uint64_t> &Opcodes,
----------------
As this is function local, can it be `static` too?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2058-2059
+                              SmallVectorImpl<Value *> &AdditionalValues) {
+  // Only salvage debug info for IcmpInst that have at least one constant
+  // integer operand, to prevent the explosion of the DIExpression.
+  auto *ConstInt = dyn_cast<ConstantInt>(Icmp->getOperand(1));
----------------
Possibly I've missed something here, but this statement (only salvage icmps with a constant integer operand) doesn't seem to be true, as the second dbg.value in the test you're adding salvages many icmps with Value operands:

    %icmp18 = icmp slt i32 %icmp17.1, %b, !dbg !15
    %icmp19 = icmp ule i32 %icmp18.1, %a, !dbg !15
    %icmp20 = icmp sle i32 %icmp19.1, %a, !dbg !15

and the corresponding CHECK line has a lot Value inputs in the DIArgList.


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

https://reviews.llvm.org/D150216