Page MenuHomePhabricator

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

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

Details

Summary

In order for the hot/cold splitting pass to graduate out of experimental
status, users need some way to safely enable it.

The current method of passing -mllvm -hot-cold-split=true to clang is
not safe, because directly setting a cl::opt cannot interact well with
other CC1 options (e.g. -O0, or -disable-llvm-passes).

This patch adds -f[no-]split-cold-code CC1 options to clang so that the
splitting pass can be toggled/combined with other options without issue.
This makes it possible to deploy the new pass on a large scale.

I've held off on adding a driver option because the ultimate goal is to
remove these options, and to simply have the pass enabled by default.

Diff Detail

Event Timeline

vsk created this revision.Fri, Jan 25, 4:28 PM
vsk updated this revision to Diff 183661.Fri, Jan 25, 4:41 PM
  • Fix a think-o in a comment.
This revision is now accepted and ready to land.Wed, Feb 6, 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.Wed, Feb 6, 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
1331

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

Herald added a project: Restricted Project. · View Herald TranscriptFri, Feb 8, 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.Mon, Feb 11, 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
1331

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.