This is an archive of the discontinued LLVM Phabricator instance.

Streamline the API of salvageDebugInfoImpl (NFC)
ClosedPublic

Authored by aprantl on Aug 3 2021, 11:49 AM.

Details

Summary

This patch refactors / simplifies salvageDebugInfoImpl(). The goal here is to simplify the implementation of coro::salvageDebugInfo() in a followup patch.

  1. Change the return value to I.getOperand(0). Currently users of salvageDebugInfoImpl() assume that the first operand is I.getOperand(0). This patch makes this information explicit. A nice side-effect of this change is that it allows us to salvage expressions such as add i8 1, %a in the future.
  2. Factor out the creation of a DIExpression and return an array of DIExpression operations instead. This change allows users that call salvageDebugInfoImpl() in a loop to avoid the costly creation of temporary DIExpressions and to defer the creation of a DIExpression until the end.

This patch does not change any functionality.

Diff Detail

Unit TestsFailed

Event Timeline

aprantl created this revision.Aug 3 2021, 11:49 AM
aprantl requested review of this revision.Aug 3 2021, 11:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2021, 11:49 AM
aprantl updated this revision to Diff 364822.Aug 6 2021, 9:26 AM

Fix a use-after-free of Op0 introduced in the previous revision and remove the redundant check for Ops.empty().

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

llvm/include/llvm/Transforms/Utils/Local.h
296

Should this be nullptr rather than false?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1286

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.

1291–1292

It might make sense to move the if (!V) check above to underneath this comment? It doesn't matter much though.

llvm/lib/Transforms/Utils/Local.cpp
1758

Do we still need the SalvagedExpr check here - I'm not sure it will ever be nullptr now?

1759

How did you choose the SmallVector size value of 16? I don't think it matters for the review, I am just curious.

aprantl added inline comments.Aug 10 2021, 2:43 PM
llvm/include/llvm/Transforms/Utils/Local.h
296

Thanks!

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1286

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.

1291–1292

Yes.

aprantl added inline comments.Aug 10 2021, 2:52 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1286

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.

llvm/lib/Transforms/Utils/Local.cpp
1758

Same comment as above, there is unfortunately no guarantee that the expressions from dbg.value intrinsics are non-null.

1759

I picked a reasonably large, round number without any statistics to back up my choice :-)

This revision was not accepted when it landed; it landed in state Needs Review.Aug 10 2021, 3:21 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.