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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

Thanks for the update, changes look good (with some minor comments inline)!

In D150216#4342620 <https://reviews.llvm.org/D150216#4342620>, @aprantl wrote:

> Thanks, I think all of my concerns have been addressed!
> Does lib/CodeGen/AsmPrinter/DwarfExpression.cpp need to be updated for the new opcodes or will just work?

That's a good point - currently these ops will be unhandled and trigger an assert in `DwarfExpression::addExpression`; after fixing that up, it'd probably be a good idea to add one or more extra tests to exercise ISel and DWARF emission. You could model the tests off of `llvm\test\DebugInfo\X86\debug_value_list_selectiondag.ll` and `llvm\test\DebugInfo\X86\dbg_value_list_emission.mir`, or add additional debug values to existing tests to cover them.



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2018-2019
     Opcodes.append({dwarf::DW_OP_constu, Val});
-  } else {
-    if (!CurrentLocOps) {
-      Opcodes.append({dwarf::DW_OP_LLVM_arg, 0});
-      CurrentLocOps = 1;
-    }
-    Opcodes.append({dwarf::DW_OP_LLVM_arg, CurrentLocOps});
-    AdditionalValues.push_back(BI->getOperand(1));
-  }
+  } else
+    handleSSAValueOperands(CurrentLocOps, Opcodes, AdditionalValues, BI);
 
----------------
Very small point, but the [[ https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements | coding standards recommend ]] braces to be used for `else` if the corresponding `if` is using them.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2072-2073
+    Opcodes.push_back(Val);
+  } else
+    handleSSAValueOperands(CurrentLocOps, Opcodes, AdditionalValues, Icmp);
+
----------------
Ditto the above comment.


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

https://reviews.llvm.org/D150216