Skip to content

Conversation

@rlavaee
Copy link
Contributor

@rlavaee rlavaee commented Nov 20, 2024

This PR allows mixing -basic-block-sections with -enable-machine-function-splitter. The strategy is to let -basic-block-sections take precedence over functions with profiles.

Comment on lines +132 to +134
// Do not split functions when -basic-block-sections=all is specified.
if (MF.getTarget().getBBSectionsType() == llvm::BasicBlockSection::All)
return false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you intend to support this combination in the future? I think basic block sections is useful for mapping in general beyond it's use in Propeller. If I recall correctly @boomanaiden154 mentioned this type of usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This applies for AutoFDO builds. The idea is to apply Propeller for functions whose BasicBlockAddressMap structure doesn't change since the profile was collected (even though those functions may have been optimized by autofdo).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For mapping, we just use the basic block address map rather than basic block sections as we only need to know where blocks are and their size rather than any ability to rearrange/GC them in the linker.

@rlavaee rlavaee force-pushed the propeller-mfs branch 3 times, most recently from 7bf6b0d to 39d8195 Compare November 21, 2024 23:00
@rlavaee
Copy link
Contributor Author

rlavaee commented Nov 21, 2024

Thanks for the quick review!

Copy link

@snehasish snehasish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rlavaee rlavaee merged commit 68f7b07 into llvm:main Nov 23, 2024
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants