[PATCH] D107383: Streamline the API of salvageDebugInfoImpl (NFC)
Adrian Prantl via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 10 14:52:10 PDT 2021
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
More information about the llvm-commits
mailing list