This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef] Use a cache to avoid creating redundant DBG_PHIs
ClosedPublic

Authored by jmorse on Apr 27 2022, 3:41 AM.

Details

Summary

In SelectionDAG, DBG_PHI instructions are created to "read" physreg values and give them an instruction number, when they can't be traced back to a defining instruction. The most common scenario is arguments to a function. Unfortunately, if you have 100 inlined methods, each of which has the same "this" pointer, then the 100 dbg.value instructions become 100 DBG_INSTR_REFs and 100 DBG_PHIs, where only one DBG_PHI would suffice.

This patch adds a vreg cache for MachineFunction::salvageCopySSA, if we've already traced a value back to the start of a block and create a DBG_PHI then it allows us to re-use the DBG_PHI, as well as reducing work.

I've added a test with an implicit check-not for DBG_PHIs, which I think is the best way of making sure they aren't over-created in the future, and deserves its own test file.

Diff Detail

Unit TestsFailed

Event Timeline

jmorse created this revision.Apr 27 2022, 3:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2022, 3:41 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jmorse requested review of this revision.Apr 27 2022, 3:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2022, 3:41 AM
Orlando accepted this revision.Apr 29 2022, 12:30 AM

Makes sense, LGTM. I've added two tiny suggestions inline but I don't mind if you leave the patch as it is.

llvm/include/llvm/CodeGen/MachineFunction.h
534

I think something like DbgPHICache might be clearer, but this is not a very strong opinion.

llvm/lib/CodeGen/MachineFunction.cpp
1042

Possibly worth adding assert(MI.isCopyLike())? Not a strong opinion.

This revision is now accepted and ready to land.Apr 29 2022, 12:30 AM
jmorse marked 2 inline comments as done.May 3 2022, 1:55 AM
jmorse added inline comments.
llvm/include/llvm/CodeGen/MachineFunction.h
534

Sounds good,

llvm/lib/CodeGen/MachineFunction.cpp
1042

IMHO no, there are assertions down both sides of the branch below that are effectively that assertion.

This revision was landed with ongoing or failed builds.May 3 2022, 1:56 AM
This revision was automatically updated to reflect the committed changes.
jmorse marked 2 inline comments as done.