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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
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
----------------
Orlando wrote:
> Should this be `nullptr` rather than `false`?
Thanks!


================
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);
----------------
Orlando wrote:
> 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.
I only threw in the check because DbgIntrinsicInst::getExpression() returns a DIExpression* and I wasn't sure whether that meant the expression is guaranteed to be non-null. I wish LLVM would use references in APIs to make this unambiguous. It's probably not necessary, and I'm happy to remove it.


================
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
----------------
Orlando wrote:
> It might make sense to move the `if (!V)` check above to underneath this comment? It doesn't matter much though.
Yes.


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

https://reviews.llvm.org/D107383