[PATCH] D107383: Streamline the API of salvageDebugInfoImpl (NFC)

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

LGTM, I just have a few small inline questions and nits.



================
Comment at: llvm/include/llvm/Transforms/Utils/Local.h:296
+/// Given an instruction \p I and DIExpression \p DIExpr operating on it, append
+/// the effects of \p I to the DIExpression operand list \p Ops, or return false
+/// if it cannot be salvaged. \p CurrentLocOps is the number of SSA values
----------------
Should this be `nullptr` rather than `false`?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1286
     SmallVector<Value *, 4> AdditionalValues;
-    DIExpression *SalvagedExpr =
-        salvageDebugInfoImpl(VAsInst, Expr, StackValue, 0, AdditionalValues);
+    V = salvageDebugInfoImpl(VAsInst, Expr ? Expr->getNumLocationOperands() : 0,
+                             Ops, AdditionalValues);
----------------
When is `Expr` nullptr here (and for the other `salvageDebugInfoImpl` calls where you make a similar check)? I may be missing something but it looks like the `Expr?` check wasn't necessary before.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1291-1292
 
     // If we cannot salvage any further, and haven't yet found a suitable debug
     // expression, bail out.
     // TODO: If AdditionalValues isn't empty, then the salvage can only be
----------------
It might make sense to move the `if (!V)` check above to underneath this comment? It doesn't matter much though.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1758
     auto LocItr = find(DIILocation, &I);
     while (SalvagedExpr && LocItr != DIILocation.end()) {
+      SmallVector<uint64_t, 16> Ops;
----------------
Do we still need the `SalvagedExpr` check here - I'm not sure it will ever be `nullptr` now?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1759
     while (SalvagedExpr && LocItr != DIILocation.end()) {
+      SmallVector<uint64_t, 16> Ops;
       unsigned LocNo = std::distance(DIILocation.begin(), LocItr);
----------------
How did you choose the `SmallVector` size value of `16`? I don't think it matters for the review, I am just curious.


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

https://reviews.llvm.org/D107383