Page MenuHomePhabricator

[PGO][PGSO] Handle MBFIWrapper
ClosedPublic

Authored by yamauchi on Jan 24 2020, 1:49 PM.

Details

Reviewers
davidxl
Summary

Some code gen passes use MBFIWrapper to keep track of the frequency of new
blocks. This was not taken into account and could lead to incorrect frequencies
as MBFI silently returns zero frequency for unknown/new blocks.

Add a variant for MBFIWrapper in the PGSO query interface.

Depends on D73494.

Event Timeline

yamauchi created this revision.Jan 24 2020, 1:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2020, 1:49 PM

The MBFIWrapper change seems NFC, can it be extracted out first?

Split the movement of MBFIWrapper into D73494.

yamauchi updated this revision to Diff 241204.Wed, Jan 29, 9:44 AM

Rebased past D73494. PTAL.

davidxl added inline comments.Wed, Jan 29, 10:00 AM
llvm/include/llvm/Transforms/Utils/SizeOpts.h
91

Is this function necessary? It is mostly the same as the previous impl.

yamauchi marked an inline comment as done.Wed, Jan 29, 10:57 AM
yamauchi added inline comments.
llvm/include/llvm/Transforms/Utils/SizeOpts.h
91

This version takes a BlockFrequency rather than a block (BlockT). Since only MBFIWrapper, rather than BFI, has the true frequency for the block, we'd need to either (1) get the frequency out of MBFIWrapper and then pass it here, or (2) pass MBFIWrapper down here. The current approach is (1). Either way, we'd need to take the frequency out of MBFIWrapper somwhere along the way, and it won't likely dedup the code as they both require a new param type that doesn't seem to be amenable to additional templatization on top of this already templatized code.

Do you see a better idea?

davidxl added inline comments.Wed, Jan 29, 11:16 AM
llvm/include/llvm/Transforms/Utils/SizeOpts.h
91

How about changing BlockT template parameter into BlockTOrBlockFreq, so that it can be instantiated with BlockFrequency too?

yamauchi updated this revision to Diff 241556.Thu, Jan 30, 12:56 PM
yamauchi marked an inline comment as done.

Addressed comment. PTAL.

llvm/include/llvm/Transforms/Utils/SizeOpts.h
91

That just worked out, except for the assert on the BB pointer non-nullness, which I resolved by moving it to the callers. Done.

davidxl accepted this revision.Thu, Jan 30, 1:24 PM

lgtm

This revision is now accepted and ready to land.Thu, Jan 30, 1:24 PM
yamauchi closed this revision.Tue, Feb 4, 10:24 AM