This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Refactor MBB hotness/coldness into templated PSI functions
ClosedPublic

Authored by shenhan on Jun 12 2023, 4:40 PM.

Details

Summary

[NFC] In D152399, we calculate BPI->BFI in MachineFunctionSplit pass just to use PSI->isFunctionHotInCallGraph, which is expensive. Instead, we can implement this directly with MBFI.

Reviewer @wenlei mentioned in his commet, that machine_size_opts already has isFunctionColdInCallGraph, isFunctionHotInCallGraphNthPercentile, etc implemented. These can be refactored and reused across MFS and machine size opts.

This CL does this - it refactors out those internal static functions into PSI as templated functions, so they can be accessed easily.

Diff Detail

Event Timeline

shenhan created this revision.Jun 12 2023, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2023, 4:40 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
shenhan requested review of this revision.Jun 12 2023, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2023, 4:40 PM
shenhan retitled this revision from [NFC] Refact MBB hotness/coldness into MBFI. to [NFC] Refactor MBB hotness/coldness into MBFI..

Thanks for the follow up. It doesn't feel like this belongs to MBFI either. BFI for IR doesn't contain any of the hotness query - instead these are all part of PSI.

I'm wondering if we can make some template functions in PSI that cover both IR and MIR hotness queries, since BFI and MBFI have very similar APIs. We have precedence for PGO code sharing between IR and MIR (SampleProfileLoaderBaseImpl<BT>).

shenhan updated this revision to Diff 531134.Jun 13 2023, 6:29 PM
shenhan updated this revision to Diff 531140.Jun 13 2023, 6:39 PM

Thanks for the follow up. It doesn't feel like this belongs to MBFI either. BFI for IR doesn't contain any of the hotness query - instead these are all part of PSI.

I'm wondering if we can make some template functions in PSI that cover both IR and MIR hotness queries, since BFI and MBFI have very similar APIs. We have precedence for PGO code sharing between IR and MIR (SampleProfileLoaderBaseImpl<BT>).

Thanks. That makes sense. I've reverted all changes to MBFI. And moved the code to PSI with templates to accommodate Function/MachineFunction, BasicBlock/MachineBasicBlock, BFI/MBFI. Also moved some function implementations from the cpp file to the header, as they are templated. (I have not chosen to template the whole PSI class, because currently I see limited usage for such changes. (And the scope of template-PSI would require a serials of CLs.)).

wenlei added a subscriber: davidxl.Jun 13 2023, 7:58 PM
llvm/include/llvm/Analysis/ProfileSummaryInfo.h
138

This is better down through template specialization. Check how SampleProfileInference<BT>::findUnlikelyJumps handles Invoke in IR.

You can extract only this part into a small helper, and specialize that helper based on template type (i.e. check call/invoke for IR, no-op for MIR)

162

same here

287

same here

291

same here

llvm/lib/Analysis/ProfileSummaryInfo.cpp
98–100

nit: this comment seem lost?

davidxl added inline comments.Jun 13 2023, 9:50 PM
llvm/include/llvm/Analysis/ProfileSummaryInfo.h
138

Agree. If we plan to have this for MBFI in the future, we can just extract this part out as a helper function too.

163

Can this part share code with the Hot checking counter part?

shenhan updated this revision to Diff 531175.Jun 13 2023, 10:39 PM
shenhan marked 4 inline comments as done.
shenhan retitled this revision from [NFC] Refactor MBB hotness/coldness into MBFI. to [NFC] Refactor MBB hotness/coldness into templated PSI functions.
shenhan edited the summary of this revision. (Show Details)
shenhan marked 3 inline comments as done.
shenhan added inline comments.
llvm/include/llvm/Analysis/ProfileSummaryInfo.h
138

Done.

163

Done by extracting part of this into templated function "findTotalCallCount", leaving hot/cold checking part unchanged. This keeps code simple and also easy to understand.

shenhan marked an inline comment as done.Jun 14 2023, 9:05 AM

Build failure is caused by one time out case:
Timed Out Tests (1):

MLIR :: Examples/standalone/test.toy

Unlikely to be caused by this change. Rerunning locally and will retry the bot later.

wenlei added inline comments.Jun 14 2023, 4:26 PM
llvm/include/llvm/Analysis/ProfileSummaryInfo.h
277

Call and Invoke are specific to IR version, so checking that in generic implementation is a bit weird. I'd suggest let generic implementation return std::nullopt, and have IR version check on Call and Invoke.

277

also nit: findTotalCallCount->getTotalCallCount to be consistent with getProfileCount.

shenhan updated this revision to Diff 531599.Jun 14 2023, 8:50 PM
shenhan marked 2 inline comments as done.
shenhan added inline comments.
llvm/include/llvm/Analysis/ProfileSummaryInfo.h
277

Good point. Done.

shenhan updated this revision to Diff 533403.Jun 21 2023, 2:27 PM
wenlei added inline comments.Jun 23 2023, 10:51 AM
llvm/lib/CodeGen/MachineSizeOpts.cpp
28

Now with PSI directly supporting IR and MIR, do we still need MachineBasicBlockBFIAdapter?

This function below (main use of adapter) can now interface with templated PSI APIs (isHotBlockNthPercentile/isColdBlockNthPercentile/isColdBlock) directly without going through adapter.

template<typename AdapterT, typename BlockTOrBlockFreq, typename BFIT>
bool shouldOptimizeForSizeImpl(BlockTOrBlockFreq BBOrBlockFreq, ProfileSummaryInfo *PSI,
                               BFIT *BFI, PGSOQueryType QueryType)
shenhan updated this revision to Diff 534089.Jun 23 2023, 3:11 PM
shenhan marked an inline comment as done.Jun 23 2023, 3:15 PM
shenhan added inline comments.
llvm/lib/CodeGen/MachineSizeOpts.cpp
28

Thanks. Good hint.

Removed "AdapterT" template-parameter from "shouldOptimizeForSizeImpl" and "shouldFuncOptimizeForSizeImpl".

This also leads to the removal of "MachineBasicBlockBFIAdapter" class. Now MachineSizeOpts.cpp looks neat.

shenhan marked an inline comment as done.Jun 23 2023, 3:19 PM
shenhan added inline comments.
llvm/include/llvm/Analysis/ProfileSummaryInfo.h
128

Also renamed all occurrences of template type name "FreqInfoType" -> "BFIT", so this is in accordance with existing code.

shenhan updated this revision to Diff 534161.Jun 23 2023, 10:45 PM

Previous build bot failure was caused by 31af18bccea95fe1ae8aa2c51cf7c8e92a1c208e, which was later reverted by c9a8a0e8a9b267fae0352af352e9c735fef019f7. Synced this patch past problematic CLs.

shenhan updated this revision to Diff 534376.Jun 25 2023, 2:33 PM
wenlei accepted this revision.Jun 25 2023, 4:27 PM

Thanks for the refactoring and cleanup. LGTM.

llvm/include/llvm/Analysis/ProfileSummaryInfo.h
127–145

nit: FunctionType->FuncT to be consistent with BFIT and other template definitions.

This revision is now accepted and ready to land.Jun 25 2023, 4:27 PM
shenhan updated this revision to Diff 534825.Jun 26 2023, 9:05 PM
shenhan marked an inline comment as done.

Thanks @wenlei for the review.

llvm/include/llvm/Analysis/ProfileSummaryInfo.h
127–145

Done for all occurrences.

shenhan retitled this revision from [NFC] Refactor MBB hotness/coldness into templated PSI functions to [Analysis] Refactor MBB hotness/coldness into templated PSI functions.Jun 26 2023, 9:16 PM
shenhan edited the summary of this revision. (Show Details)
shenhan retitled this revision from [Analysis] Refactor MBB hotness/coldness into templated PSI functions to [NFC] Refactor MBB hotness/coldness into templated PSI functions.Jun 26 2023, 9:18 PM
shenhan edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Jun 26 2023, 9:58 PM
This revision was automatically updated to reflect the committed changes.
shenhan marked an inline comment as done.

This seems to have broken quite a few builders. Do you mind reverting if you don't have a quick fix available?