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

[Alex Langford via Phabricator via llvm-commits]

bulbazord added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2056
+  // Values wider than 64 bits cannot be represented within a DIExpression.
+  if (ConstInt && ConstInt->getBitWidth() > 64)
+    return nullptr;
----------------
nit: you can drop the check for `ConstInt` in this condition because it must be valid due to the above check.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2064
+  Opcodes.append({dwarf::DW_OP_LLVM_arg, CurrentLocOps});
+  AdditionalValues.push_back(Icmp->getOperand(1));
+
----------------
nit: You could `push_back(ConstInt)` since it's the same thing you already computed instead of calling `Icmp->getOperand(1)` again.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2068-2070
+  uint64_t DwarfIcmpOp = getDwarfOpForIcmpPred(Icmp->getPredicate());
+  if (!DwarfIcmpOp)
+    return nullptr;
----------------
You've already changed `AdditionalValues` by the time you get to this point... Do the callers verify that the return value of this function is `nullptr` before using `AdditionalValues`? Even if they do today, somebody may use this function without realizing that this is the behavior with surprising results. My suggestion would be to only mutate the output references (`Opcodes` and `AdditionalValues`) when you know that this function will succeed.


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

https://reviews.llvm.org/D150216