Page MenuHomePhabricator

[PM/CC1] Add -f[no-]split-cold-code CC1 options to toggle splitting
AcceptedPublic

Authored by vsk on Jan 25 2019, 4:28 PM.

Details

Summary

[PM/CC1] Add -f[no-]split-cold-code CC1 options to toggle splitting

This patch adds -f[no-]split-cold-code CC1 options to clang. This allows
the splitting pass to be toggled on/off. The current method of passing
-mllvm -hot-cold-split=true to clang isn't ideal as it may not compose
correctly (say, with -O0 or -Oz).

To implement the -fsplit-cold-code option, an attribute is applied to
functions to indicate that they may be considered for splitting. This
removes some complexity from the old/new PM pipeline builders, and
behaves as expected when LTO is enabled.

Diff Detail

Event Timeline

vsk created this revision.Jan 25 2019, 4:28 PM
vsk updated this revision to Diff 183661.Jan 25 2019, 4:41 PM
  • Fix a think-o in a comment.
This revision is now accepted and ready to land.Feb 6 2019, 11:08 AM

Thanks @hiraditya. I'd also like to get a +1 from someone who's worked on the NewPM infrastructure extensively, just to make sure those changes look ok.

vsk updated this revision to Diff 185642.Feb 6 2019, 2:16 PM

Rebase.

Overall it looks ok to me, but I'd like Chandler to comment regarding the preferred way to do this with the new PM, since we don't tend to use booleans there in the PassBuilder to control passes. Is it preferable to instead use a new function attribute instead of boolean flags on the PMs (e.g. the way -fno-inline is handled)?

clang/lib/Frontend/CompilerInvocation.cpp
1340

would it be appropriate to give a warning when being ignored?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 9:34 AM

I'd prefer not adding this kind of state to PassBuilder. SplitColdCode is soemthing that refers to the construction of one particular pipeline, not to pipeline-building in general. It should be an argument passed down through the build*Pipeline calls.

vsk marked an inline comment as done.Feb 11 2019, 11:39 AM

I'd prefer not adding this kind of state to PassBuilder. SplitColdCode is soemthing that refers to the construction of one particular pipeline, not to pipeline-building in general. It should be an argument passed down through the build*Pipeline calls.

I'm not sure I understand the distinction being made here -- could you elaborate? Isn't enabling a pass related to pipeline-building in general? If not, I don't see how arguments to build*Pipeline do become related to pipeline-building in general.

For context, I've modeled the addition of the SplitColdCode member to PassBuilder on PGOOpt (c.f. PGOOptions::RunProfileGen). One thing I like about this approach is that it doesn't require changing IR, or changing very many different APIs. But if it's really not reasonable, I'm happy to take another shot at it.

clang/lib/Frontend/CompilerInvocation.cpp
1340

I think so, I'll add this in an update.

In D57265#1393453, @vsk wrote:

I'd prefer not adding this kind of state to PassBuilder. SplitColdCode is soemthing that refers to the construction of one particular pipeline, not to pipeline-building in general. It should be an argument passed down through the build*Pipeline calls.

I'm not sure I understand the distinction being made here -- could you elaborate? Isn't enabling a pass related to pipeline-building in general? If not, I don't see how arguments to build*Pipeline do become related to pipeline-building in general.

For context, I've modeled the addition of the SplitColdCode member to PassBuilder on PGOOpt (c.f. PGOOptions::RunProfileGen). One thing I like about this approach is that it doesn't require changing IR, or changing very many different APIs. But if it's really not reasonable, I'm happy to take another shot at it.

I cant say for PGOOpt in particular, but overall idea of PassBuilder is that, well, it is not "builder" as per "builder pattern".
You do not fill it with parts of a complex pipeline object and then press a magical "build" button.
Instead you ask it to build various "named" pipelines, or just parse it from text.
If you compare with PassManagerBuilder, which contains a dozen of various data members that seem to drive the pipeline construction,
PassBuilder contains only a few. And the purpose of PassBuilder members is to be used during individual *pass*es creation.

Say, you wont find OptLevel there. Instead it is being passed as an argument to build*Pipeline functions.

As for PGOOpts - it seems to be the only member that stands out.
And to me it looks like we should remove it from PassBuilder either, and start passing it to build*Pipeline functions as well.
But honestly, this is the first time I really looked through the PGOOpts code, so I might be well mistaken.

vsk added a comment.Mar 22 2019, 3:38 PM
In D57265#1393453, @vsk wrote:

I'd prefer not adding this kind of state to PassBuilder. SplitColdCode is soemthing that refers to the construction of one particular pipeline, not to pipeline-building in general. It should be an argument passed down through the build*Pipeline calls.

I'm not sure I understand the distinction being made here -- could you elaborate? Isn't enabling a pass related to pipeline-building in general? If not, I don't see how arguments to build*Pipeline do become related to pipeline-building in general.

For context, I've modeled the addition of the SplitColdCode member to PassBuilder on PGOOpt (c.f. PGOOptions::RunProfileGen). One thing I like about this approach is that it doesn't require changing IR, or changing very many different APIs. But if it's really not reasonable, I'm happy to take another shot at it.

I cant say for PGOOpt in particular, but overall idea of PassBuilder is that, well, it is not "builder" as per "builder pattern".
You do not fill it with parts of a complex pipeline object and then press a magical "build" button.
Instead you ask it to build various "named" pipelines, or just parse it from text.
If you compare with PassManagerBuilder, which contains a dozen of various data members that seem to drive the pipeline construction,
PassBuilder contains only a few. And the purpose of PassBuilder members is to be used during individual *pass*es creation.

Say, you wont find OptLevel there. Instead it is being passed as an argument to build*Pipeline functions.

As for PGOOpts - it seems to be the only member that stands out.
And to me it looks like we should remove it from PassBuilder either, and start passing it to build*Pipeline functions as well.
But honestly, this is the first time I really looked through the PGOOpts code, so I might be well mistaken.

Thanks everyone for your patient feedback. I think I understand the advantages of keeping state out of the pipeline builder now.

Teresa's earlier suggestion to use an IR attribute seems like a promising approach. In particular, it makes it simple to correctly enable/disable splitting during LTO. I'll try to post an updated patch soon.

vsk updated this revision to Diff 192210.Mon, Mar 25, 2:42 PM
vsk edited the summary of this revision. (Show Details)

Use a function attribute to implement the -fsplit-cold-code option. This removes some complexity from the old/new PMs and ensures consistent behavior when LTO is enabled.