Page MenuHomePhabricator

[Driver] Add flags for enabling both types of PGO Instrumentation
ClosedPublic

Authored by jakev on Jun 28 2016, 5:20 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jakev updated this revision to Diff 62138.Jun 28 2016, 5:20 PM
jakev retitled this revision from to [Driver] Add flags for enabling both types of PGO Instrumentation.
jakev updated this object.
jakev added reviewers: silvas, davidxl, friss, vsk, bob.wilson, xur.
jakev set the repository for this revision to rL LLVM.
jakev added a subscriber: cfe-commits.
silvas edited edge metadata.Jun 28 2016, 10:04 PM

Some basic comments.

include/clang/Driver/Options.td
483 ↗(On Diff #62138)

This help text is stale.

486 ↗(On Diff #62138)

You should probably update this help text to say something about applying.

lib/Driver/Tools.cpp
3474 ↗(On Diff #62138)

Call this PGOTrainArg for consistency.

3496 ↗(On Diff #62138)

Call this PGOApplyArg for consistency.

vsk edited edge metadata.Jun 28 2016, 10:09 PM

This lgtm with one nit, and pending approval from others.

include/clang/Driver/Options.td
482 ↗(On Diff #62138)

Would something like <options> be a better MetaVarName?

jakev updated this revision to Diff 62282.Jun 29 2016, 1:47 PM
jakev edited edge metadata.
jakev marked 5 inline comments as done.

Address comments.

xur edited edge metadata.Jun 29 2016, 1:49 PM

Thanks for doing this. I like the patch in general. Hers are my minor feedbacks:
(1) It seems the original options of -fprofile-instr-generate and -fprofile-instr-use will be kept. Is so, what is the guideline to use these two sets of options. Also, we need some documentations for the new user-visible
options.
(2) One difference b/w -fprofile-instr-generate/-fprofile-instr-use and the new options is -fpgo-train={source-cfg | optimizer-cfg} can coexist with -fpgo-apply=<file>. I don't think they work as intended in current implementation. I would suggest to not allow it.
(3) I would request a driver option to specify the profile name. While LLVM_PROFILE_FILE environment variable can do that, a command line option is gonna be useful (at least in our setup).

-Rong

jakev updated this revision to Diff 62527.Jul 1 2016, 1:39 PM
jakev edited edge metadata.

Don't allow -fpgo-train and -fpgo-apply together. Add -fpgo-train-default-output=* to set the default profile output file.

jakev added a comment.Jul 1 2016, 1:41 PM
In D21823#470516, @xur wrote:

(1) It seems the original options of -fprofile-instr-generate and -fprofile-instr-use will be kept. Is so, what is the guideline to use these two sets of options.

Sorry I'm not sure I understand what you're asking, but it's my understanding that we would simply detail the lack of interoperability between the existing set of flags and the ones added by this patch.

Also, we need some documentations for the new user-visible
options.

I'm happy to add some info to the Clang UserManual in this patch if everyone is happy with the flag names.

(2) One difference b/w -fprofile-instr-generate/-fprofile-instr-use and the new options is -fpgo-train={source-cfg | optimizer-cfg} can coexist with -fpgo-apply=<file>. I don't think they work as intended in current implementation. I would suggest to not allow it.

Sure, I've updated the patch to not allow this.

jakev updated this object.Jul 1 2016, 1:42 PM
davidxl added inline comments.Jul 1 2016, 2:22 PM
include/clang/Driver/Options.td
507 ↗(On Diff #62527)

Why is it called 'default' output? The default output is the one used why user does not specify any name.

Why not just -fpgo-train-output=... ?

lib/Driver/Tools.cpp
3560 ↗(On Diff #62527)

I think it is better to make the selector 'source' vs 'cfg'.

-fpgo-train=source
-fpgo-train=cfg

silvas added inline comments.Jul 1 2016, 4:32 PM
lib/Driver/Tools.cpp
3560 ↗(On Diff #62527)

So would -fpgo-train=cfg enable icall instrumentation or not?

My thinking is that down the road we will have one flag for each independent instrumentation capability (and of course some are mutually incompatible). This mirrors what the sanitizers do. For example, we would have:
-fpgo-train=optimizer-cfg --> IR edge profiling
-fpgo-train=optimizer-icall --> IR icall value profiling
-fpgo-train=optimizer-... --> other independent instrumentation we can do in IR instrumentation.
-fpgo-train=source-cfg --> FE edge profiling
-fpgo-train=source-icall --> FE icall profiling (if that even exists; I see some code but there is no user-visible flag)
-fpgo-train=source-... --> other FE instrumentation.

We can then have -fpgo-train=optimizer enable e.g. -fpgo-train=optimizer-cfg,optimizer-icall.
We can also have -fpgo-train=source enable e.g. -fpgo-train=source-cfg.

Since instrumentation can have different overheads or different runtime requirements, users may want to disable some instrumentation.

jakev updated this revision to Diff 63367.Jul 8 2016, 6:27 PM
jakev updated this object.
jakev marked 3 inline comments as done.

Reduce instrumentation flags to fpgo-train and change profile output file flag to fpgo-train-output.

davidxl edited edge metadata.Jul 9 2016, 4:01 PM

I should have brought it up earlier, but I forgot. I think a better (and simpler) proposal is to make -fprofile-generate and -fprofile-use turn on IR based PGO.

-fprofile-generate and -fprofile-use options were introduced by Diego (cc'ed) recently for GCC option compatibility. At that time, IR based PGO was not yet ready, so they were made aliases to FE instrumentation options -fprofile-instr-generate and -fprofile-instr-use. Now I think it is time to make it right. The documentation only says that these two options are gcc compatible options to control profile generation and use, but does not specify the underlying implementation. It is also likely that Google is the only user of this option. If a user using this option really want FE instrumentation, there is always -fprofile-instr-generate to use. It also more likely that IR instrumentation is what user wants when using GCC compatible options (as they behave more similarly).

lib/Driver/Tools.cpp
3493 ↗(On Diff #63367)

Is this check needed?

3513 ↗(On Diff #63367)

Merge this with ProfileUseArg.

3531–3532 ↗(On Diff #63367)

not needed

3535 ↗(On Diff #63367)

not needed

3539 ↗(On Diff #63367)

not needed

3543 ↗(On Diff #63367)

if ((ProfileGenerateArg || ProfileTrainArg) && ProfileUseArg)

3568 ↗(On Diff #63367)

Not needed.

silvas added a comment.Jul 9 2016, 4:39 PM

I should have brought it up earlier, but I forgot. I think a better (and simpler) proposal is to make -fprofile-generate and -fprofile-use turn on IR based PGO.

-fprofile-generate and -fprofile-use options were introduced by Diego (cc'ed) recently for GCC option compatibility. At that time, IR based PGO was not yet ready, so they were made aliases to FE instrumentation options -fprofile-instr-generate and -fprofile-instr-use. Now I think it is time to make it right. The documentation only says that these two options are gcc compatible options to control profile generation and use, but does not specify the underlying implementation. It is also likely that Google is the only user of this option. If a user using this option really want FE instrumentation, there is always -fprofile-instr-generate to use. It also more likely that IR instrumentation is what user wants when using GCC compatible options (as they behave more similarly).

This SGTM.

It may cause some user confusion since there is no obvious distinction between "profile generate" and "profile instr generate" from a user perspective. But we can avoid that with improved documentation.

My main concern is that the existing documentation already says that -fprofile-generate behaves identically to -fprofile-instr-generate http://clang.llvm.org/docs/UsersManual.html#cmdoption-fprofile-generate
However, I think it is reasonable to change this behavior (and of course the documentation), as users of this option are likely using it for compatibility with GCC and so they likely don't care about the specifics of clang FEPGO.
We probably want to at least leave a small note in the documentation regarding this change of behavior.

jakev updated this revision to Diff 63634.Jul 11 2016, 7:55 PM
jakev updated this object.
jakev edited edge metadata.

Change patch to use -fprofile-generate to enable IRPGO.

Please also update user manual: docs/UserManual.rst

lib/Driver/Tools.cpp
3596 ↗(On Diff #63634)

This should probably split out as a follow up.

test/Driver/clang_f_opts.c
90 ↗(On Diff #63634)

Probably change this test into the check of disallowed option combination : -fprofile-instr-generate -fprofile-generate

test/Profile/gcc-flag-compatibility.c
10 ↗(On Diff #63634)

Please add a FIXME here. I noticed that there is a bug in IR PGO implementation that -fprofile-generate still invokes overrider api in instrumentation which is unnecessary.

jakev updated this revision to Diff 64056.Jul 14 2016, 3:46 PM
jakev marked 3 inline comments as done.

Add a couple notes to the docs and a fixme to a test. We can more thoroughly fix up the docs in a separate patch.

test/Profile/gcc-flag-compatibility.c
10 ↗(On Diff #63634)

I actually have a patch that fixes this behavior. I can post it once this patch has landed.

silvas accepted this revision.Jul 15 2016, 4:36 PM
silvas edited edge metadata.

Once all of David's comments are addressed this LGTM.

This revision is now accepted and ready to land.Jul 15 2016, 4:36 PM
davidxl accepted this revision.Jul 15 2016, 4:53 PM
davidxl edited edge metadata.

lgtm

jakev added a comment.Jul 15 2016, 6:19 PM

Fantastic. Sean or David, could one of you commit this for me? It would be much appreciated.

This revision was automatically updated to reflect the committed changes.