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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
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);
----------------
aprantl wrote:
> 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.
Actually, DIBuilder::insertDbgValueIntrinsic() doesn't prevent a language frontend from creating a dbg.value with a nullptr DIExpression. And Verifier accepts it, too. If we wanted to make this guarantee then we'd need to enforce it in the Verifier first.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1758
     auto LocItr = find(DIILocation, &I);
     while (SalvagedExpr && LocItr != DIILocation.end()) {
+      SmallVector<uint64_t, 16> Ops;
----------------
Orlando wrote:
> Do we still need the `SalvagedExpr` check here - I'm not sure it will ever be `nullptr` now?
Same comment as above, there is unfortunately no guarantee that the expressions from dbg.value intrinsics are non-null.


================
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);
----------------
Orlando wrote:
> How did you choose the `SmallVector` size value of `16`? I don't think it matters for the review, I am just curious.
I picked a reasonably large, round number without any statistics to back up my choice :-)


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

https://reviews.llvm.org/D107383