This is an archive of the discontinued LLVM Phabricator instance.

[PGO] change profile use cc1 option
ClosedPublic

Authored by xur on Feb 29 2016, 1:55 PM.

Details

Summary

This patch takes the suggestion by Sean Silva in https://reviews.llvm.org/D17622 to decouple cc1 option from driver level option.

-fprofile-instr-use={} is now a driver only option. The cc1 part is replaced by -fprofile-instrument-use={llvm,clang}.
It sets a CodeGenOpt ProfileUse which is either {None, Clang, LLVM}, a common enum for -fprofile-instrument={} (for the profile instrument).

The profile path cc1 option now becomes -fprofile-instrument-usepath={}.

Diff Detail

Repository
rL LLVM

Event Timeline

xur updated this revision to Diff 49421.Feb 29 2016, 1:55 PM
xur retitled this revision from to [PGO] change profile use cc1 option.
xur updated this object.
xur added reviewers: davidxl, silvas, bogner.
xur added subscribers: davidxl, xur, cfe-commits.
silvas edited edge metadata.Feb 29 2016, 3:55 PM

Thanks, this is looking good. I have some naming suggestions and nits, but the overall content of the patch LGTM.

include/clang/Driver/CC1Options.td
286 ↗(On Diff #49421)

Small bikeshed: usepath reads weird to me. Can we make it use-path?

include/clang/Frontend/CodeGenOptions.h
237 ↗(On Diff #49421)

No space before the parens. Here and elsewhere.

lib/Frontend/CompilerInvocation.cpp
378 ↗(On Diff #49421)

Can we reuse the enum used for CodeGenOpts? Please add a comment if not.

533 ↗(On Diff #49421)

Maybe consider renaming these variables in a separate patch. The current naming is pretty confusing since it makes it seems like InstrProfileOutput mirrors InstrProfileInput, but they are totally unrelated. Maybe InstrProfileDefaultProfrawPath and InstrProfileUsePath, respectively.

Maybe in this patch rename InstrProfileInput to ProfileInstrumentUsePath to match the option name?

xur marked 4 inline comments as done.Feb 29 2016, 6:02 PM
xur added inline comments.
lib/Frontend/CompilerInvocation.cpp
378 ↗(On Diff #49421)

this is a very good suggestion. I'll change the code as you suggested.

533 ↗(On Diff #49421)

Will change the name InstrProfileInput to ProfileInstrumentUsePath in this path.

I will change Opts.InstrProfileOutput to InstrProfileDefaultProfrawPath in a separated patch.

silvas accepted this revision.Feb 29 2016, 6:06 PM
silvas edited edge metadata.

Thanks!

This revision is now accepted and ready to land.Feb 29 2016, 6:06 PM
xur updated this revision to Diff 49444.Feb 29 2016, 6:18 PM
xur edited edge metadata.
xur marked 2 inline comments as done.

Integrated Sean's review suggestions.

-Rong

Some comments but otherwise feel free to commit.

lib/CodeGen/CodeGenModule.cpp
154 ↗(On Diff #49444)

Are we ensuring consistency in the driver? If not, we definitely want to do that.

lib/Frontend/CompilerInvocation.cpp
381 ↗(On Diff #49444)

I think it would be better to assert that it is one of the two options rather than silently doing nothing.

xur marked an inline comment as done.Mar 1 2016, 10:59 AM
xur added inline comments.
lib/CodeGen/CodeGenModule.cpp
154 ↗(On Diff #49444)

I think in driver level, the change change is consistent with the old behavior: driver will always set option -fprofile-instrument-use-path= for the pgo use compilation,
i.e. when -fprofile-instru-use{=<path>} or -fprofile-use{=<path>} is in the command line, we will always have -fprofile-instrument-use-path=<path>,
It will be translated to CodeGenOpts.ProfileInstrumentUsePath in ParseCodeGenArgs().

In the cc1 level, the behavior changed: old behavior is:
-fprofile-instru-use=<path> will trigger the use compilation.
Now requires two cc1 opitons:
-fprofile-instrument=clang -fprofile-instrument-use-path=<path>.
Having -fprofile-instrument=clang only will trigger the assertion and
having -fprofile-instrument-use-path=<path> is vold op.
I did this because the old pgo use cc1 option requires the profile path.

This is contrast in the IR level instrument, where -fprofile-instrument-use-path=<path> is optional: we will use default.profdata in the absence.

lib/Frontend/CompilerInvocation.cpp
381 ↗(On Diff #49444)

Yes. Assertion is better. Will do in the commit.

davidxl edited edge metadata.Mar 1 2016, 9:50 PM

I think we just need one cc1 option -fprofile-instrument-use-path=<>. An overloaded setPGOInstrumenter method can peak at the profile header and get the Profile flavor.

xur updated this revision to Diff 49647.Mar 2 2016, 10:41 AM
xur edited edge metadata.
xur marked an inline comment as done.

I think David's suggestion to use one cc1 option -fprofile-instrument-use-path is a very good idea.
I update the patch to get rid of -fprofille-instrument-use.

The profile kind is now set by checking the profile header. I use a separated function (instead of using the overloaded name of setPGOInstrumentor()).

The assertion in lib/CodeGen/CodeGenModule.cpp:153 might not be needed. As Profilekind CodeGenOpts is getting from CodeGenOpts.ProfileInstrumentUsePath, so it should not be empty. But I keep it just in case.

I agree, David's suggestion has simplified things. This LGTM (with a couple nits) but I'll let David give the final approval.

lib/CodeGen/CodeGenModule.cpp
153 ↗(On Diff #49647)

This assertion is not needed anymore. Or at least should be reworded as "hasProfileClangUse but no ProfileInstrumentPath" or similar to clearly indicate the expected state that should be guaranteed by the rest of the program.

lib/Driver/Tools.cpp
3318 ↗(On Diff #49647)

This is not needed anymore after David's suggestion.

lib/Frontend/CompilerInvocation.cpp
404 ↗(On Diff #49647)

I think this assertion is not needed since we are already handling errors below (by deferring to clang).

406 ↗(On Diff #49647)

typo: reutrn

xur updated this revision to Diff 49655.Mar 2 2016, 11:38 AM

Integrated with Sean's review comments.

-Rong

davidxl accepted this revision.Mar 2 2016, 11:49 AM
davidxl edited edge metadata.

lgtm

This revision was automatically updated to reflect the committed changes.