This is an archive of the discontinued LLVM Phabricator instance.

[PassTiming] cleaning up legacy PassTimingInfo interface. NFCI.
ClosedPublic

Authored by fedor.sergeev on Sep 21 2018, 5:53 AM.

Details

Summary

During D51276 discussion it was decided that legacy PassTimingInfo
interface can not be reused for new pass manager's implementation
of -time-passes.

This is a cleanup in preparation for D51276 to make legacy interface
as concise as possible, moving the PassTimingInfo from the header
into the anonymous legacy namespace in .cpp.

It is rather close to a revert of rL340872 in a sense that it hides
the interface and gets rid of templates. However as compared to
a complete revert it resides in a different translation unit and has
an additional pass-instance counting funcitonality (PassIDCountMap).

Diff Detail

Repository
rL LLVM

Event Timeline

fedor.sergeev created this revision.Sep 21 2018, 5:53 AM
fedor.sergeev edited the summary of this revision. (Show Details)Sep 21 2018, 6:15 AM

is this in some sense a revert of rL340872? Which parts of that change remain?

is this in some sense a revert of rL340872? Which parts of that change remain?

It is not a complete revert, but definitely very close to it.
One of the changes that were not restored to the original form is removal of createTheTimeInfo, which had been replaced with init() and still remains in that form now.
Functionally, the biggest change from pre-L340872 was introduced a bit later - PassIDCountMap that counts instances and adds #instance-numbers to the report.
And it remains "unreverted" in this change.

Is that remaining change still useful? Does it make sense to revert instead?

Is that remaining change still useful? Does it make sense to revert instead?

At the very least we need the move from LegacyPassManager.cpp into PassTimingInfo.cpp, to consolidate different implementations of time-passes in a single source file.
You can see this change (compared to pre-rL340872) as a move with cleanup.

philip.pfaffe accepted this revision.Sep 26 2018, 2:03 AM

Okay, all in all this looks alright, and making a single cleanup change seems less verbose than reverting rL340872 first. Please make very clear in this commit message that this is a partial revert and why.

This revision is now accepted and ready to land.Sep 26 2018, 2:03 AM
fedor.sergeev edited the summary of this revision. (Show Details)Sep 26 2018, 2:31 AM
This revision was automatically updated to reflect the committed changes.