This is an archive of the discontinued LLVM Phabricator instance.

[LazyMachineBFI] Reimplement with getAnalysisIfAvailable
ClosedPublic

Authored by anemet on Feb 17 2017, 4:52 PM.

Details

Summary

Since LoopInfo is not available in machine passes as universally as in IR
passes, using the same approach for OptimizationRemarkEmitter as we did for IR
will run LoopInfo and DominatorTree unnecessarily. (LoopInfo is not used
lazily by ORE.)

To fix this, I am modifying the approach I took in D29836. LazyMachineBFI now
uses its client passes including MachineBFI itself that are available or
otherwise compute them on the fly.

So for example the GreedyRegAlloc since it's already using MBFI will reuse
that instance. On the other hand, AsmPrinter in Justin's patch will generate
DT, LI and finally BFI on the fly.

(I am of course wondering now if the simplicity of this approach is even
preferable in IR. I will do some experiments.)

Testing is provided by an updated version of D29837 which requires Justin's
patch to bring ORE to the AsmPrinter.

Diff Detail

Repository
rL LLVM

Event Timeline

anemet created this revision.Feb 17 2017, 4:52 PM
anemet edited the summary of this revision. (Show Details)Feb 17 2017, 4:56 PM
davide added a subscriber: davide.Feb 17 2017, 7:15 PM

What's the compile time savings you're seeing from not computing these analyses unnecessarily?

Side rant: I think some of the names in LLVM are really poorly chosen, DominatorTree in fact is not an analysis but the result of an analysis.

What's the compile time savings you're seeing from not computing these analyses unnecessarily?

This would be hard to measure for the machine passes since GreedyRegAlloc already uses MachineLoopInfo. This was more of a precaution for the machine passes at this point. But I can try to measure this when I look at the IR passes.

Side rant: I think some of the names in LLVM are really poorly chosen, DominatorTree in fact is not an analysis but the result of an analysis.

Agreed and then in both MachineLoopInfo and MachineDT, we go out of our way to pretend that the pass has the same APIs as the result. Very confusing.

davidxl edited edge metadata.Feb 21 2017, 10:18 AM

Is there a need to keep LazyBlockFrequencyInfo as a template class after this change?

include/llvm/CodeGen/MachineBlockFrequencyInfo.h
41 ↗(On Diff #88994)

Is there a need for this helper -- it is used only in one place. Just construct one and call calculate?

lib/CodeGen/LazyMachineBlockFrequencyInfo.cpp
61 ↗(On Diff #88994)

Set MBPI here too?

anemet marked an inline comment as done.Feb 21 2017, 11:15 AM

Is there a need to keep LazyBlockFrequencyInfo as a template class after this change?

No. I'd like to evaluate if we want to use the same approach as this in LBFI (do you have any opinion on this?) and if yes, the entire class can go.

anemet updated this revision to Diff 89252.Feb 21 2017, 11:19 AM

Address David's comments.

Hi David, is this looking good to you now? Sorry to push but Justin's change is blocked by this. Thanks!!

davidxl accepted this revision.Feb 23 2017, 9:12 AM

lgtm

This revision is now accepted and ready to land.Feb 23 2017, 9:12 AM

Thanks, David!

This revision was automatically updated to reflect the committed changes.