Page MenuHomePhabricator

[Propeller] Promote functions with propeller profiles to .text.hot.
ClosedPublic

Authored by rahmanl on Apr 1 2022, 12:17 PM.

Details

Summary

Today, text section prefixes (none, .unlikely, .hot, and .unkown) are determined based on PGO profile. However, Propeller may deem a function hot when PGO doesn't. Besides, when -Wl,-keep-text-section-prefix=true Propeller cannot enforce a global section ordering as the linker can only reorder sections within each output section (.text, .text.hot, .text.unlikely).

This patch promotes all functions with Propeller profiles (functions listed in the basic-block-sections profile) to .text.hot. The feature is hidden behind the flag --bbsections-guided-section-prefix which defaults to true.

The new implementation refactors the parsing of basic block sections profile into a new BasicBlockSectionsProfileReader analysis pass. This allows us to use the information earlier in CodeGenPrepare in order to set the functions text prefix. BasicBlockSectionsProfileReader will be used both by BasicBlockSections pass and CodeGenPrepare.

Diff Detail

Event Timeline

rahmanl created this revision.Apr 1 2022, 12:17 PM
Herald added a project: Restricted Project. · View Herald Transcript
rahmanl updated this revision to Diff 424581.Apr 22 2022, 12:37 PM

Refactor code.

rahmanl retitled this revision from Add propeller functions to their own sections. to [Propeller] Promote functions with propeller profiles to .text.hot..Apr 22 2022, 3:40 PM
rahmanl edited the summary of this revision. (Show Details)
rahmanl updated this revision to Diff 424648.Apr 22 2022, 3:57 PM

Fix comment.

rahmanl published this revision for review.Apr 25 2022, 10:40 AM
rahmanl edited the summary of this revision. (Show Details)
rahmanl added a reviewer: tmsriram.
rahmanl added subscribers: shenhan, amharc, vporpo.
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2022, 10:41 AM
rahmanl edited the summary of this revision. (Show Details)Apr 26 2022, 12:22 PM
rahmanl updated this revision to Diff 425283.Apr 26 2022, 12:25 PM

Remove unnecessary includes.

This looks good to me. One question, do we have a rough idea, what is the percentage of functions that pgo and propeller disagree about their hotness?

llvm/lib/CodeGen/CodeGenPrepare.cpp
495

Are we sure BBSectionsProfileReader here is always not null?

rahmanl updated this revision to Diff 427410.May 5 2022, 11:46 AM
rahmanl marked an inline comment as done.

Add null checking.

This looks good to me. One question, do we have a rough idea, what is the percentage of functions that pgo and propeller disagree about their hotness?

Yes. For one example we have
31% of Propeller functions in .text.hot,
57% in .text
11% in .text.unlikely

One questions is whether the promotion should be hidden behind a flag or not.

llvm/lib/CodeGen/CodeGenPrepare.cpp
495

Thanks. Added the check.

This looks good to me. One question, do we have a rough idea, what is the percentage of functions that pgo and propeller disagree about their hotness?

Yes. For one example we have
31% of Propeller functions in .text.hot,
57% in .text
11% in .text.unlikely

Thanks.

One questions is whether the promotion should be hidden behind a flag or not.

My thought is that this is a prerequisite for propeller inter-proc layout optimization, so if inter-proc layout optimization is on by default when -fbasic-block-sections=list=<...> is present, then this could also be on by default. On the other hand if we guard inter-proc layout optimization with a flag, then that flag could also cover this.

rahmanl updated this revision to Diff 429343.May 13 2022, 1:28 PM
  • Hide the feature behind the flag --bbsections-guided-section-prefix.

This looks good to me. One question, do we have a rough idea, what is the percentage of functions that pgo and propeller disagree about their hotness?

Yes. For one example we have
31% of Propeller functions in .text.hot,
57% in .text
11% in .text.unlikely

Thanks.

One questions is whether the promotion should be hidden behind a flag or not.

My thought is that this is a prerequisite for propeller inter-proc layout optimization, so if inter-proc layout optimization is on by default when -fbasic-block-sections=list=<...> is present, then this could also be on by default. On the other hand if we guard inter-proc layout optimization with a flag, then that flag could also cover this.

Thanks. To clarify, this is also needed for function ordering. I went ahead and introduced a flag for this. The feature would also work when --profile-guided-section-prefix=false.

rahmanl edited the summary of this revision. (Show Details)May 13 2022, 1:34 PM
MaskRay added inline comments.May 14 2022, 6:03 PM
llvm/include/llvm/Analysis/BasicBlockSectionsProfileReader.h
30

Delete these using llvm::*;

38
48

Check whether you want to use SmallVector<BBClusterInfo, 0> to make the element size smaller.

llvm/lib/Analysis/BasicBlockSectionsProfileReader.cpp
1

Not sure the Reader should be placed in lib/Analysis. There is no precedent.

26

delete

rahmanl updated this revision to Diff 429806.May 16 2022, 12:02 PM
rahmanl marked 5 inline comments as done.
  • Move Reader pass to CodeGen.
rahmanl added inline comments.May 16 2022, 12:09 PM
llvm/include/llvm/Analysis/BasicBlockSectionsProfileReader.h
48

How about SmallVector<BBClusterInfo> for the absence of a strongly-motivated choice as advised in https://llvm.org/doxygen/classllvm_1_1SmallVector.html ?

llvm/lib/Analysis/BasicBlockSectionsProfileReader.cpp
1

Moved to CodeGen (where we have MIRSampleProfile.cpp).

MaskRay added inline comments.May 16 2022, 12:12 PM
llvm/include/llvm/Analysis/BasicBlockSectionsProfileReader.h
48

That's also fine.

LGTM

llvm/lib/CodeGen/CodeGenPrepare.cpp
192

Maybe add more details in help message - "Other functions won't be impacted, i.e., their prefixes are decided by FDO/AutoFDO profile."

rahmanl updated this revision to Diff 431738.May 24 2022, 11:24 AM
rahmanl marked an inline comment as done.

Expand flag description.

rahmanl updated this revision to Diff 432365.May 26 2022, 1:10 PM

Added missing pipeline tests.

This revision was not accepted when it landed; it landed in state Needs Review.May 26 2022, 4:23 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
rahmanl updated this revision to Diff 432445.May 26 2022, 7:16 PM