Page MenuHomePhabricator

[PGO] clang cc1 option change to enable IR level instrumenation
ClosedPublic

Authored by xur on Feb 25 2016, 2:06 PM.

Details

Summary

This patch expands cc1 option -fprofile-instrument= with three new values:
(1) -fprofile-instrument=llvm
This enables IR PGO instrumentation.
(2) -fprofile-instrument=llvm-use
This enables PGO use compilation using IR level profiles. The profile is specified by -fprofile-instrument-path= (cc1) option.
(3) -fprofile-instrument=clang-use
This enables PGO use compilation using clang profiles. This replaces current cc1 option -fprofile-instr-use=. The profile is specified by -fprofile-instrument-path= (cc1) option.

Some tests are modified due to the -fprofile-instr-use= option change.
A new test is added to test IR profile compilation.
test/CodeGen/pgo-instrumentation.c

Original discussion is in http://reviews.llvm.org/D15829.

Diff Detail

Repository
rL LLVM

Event Timeline

xur updated this revision to Diff 49113.Feb 25 2016, 2:06 PM
xur retitled this revision from to [PGO] clang cc1 option change to enable IR level instrumenation.
xur updated this object.
xur added reviewers: davidxl, silvas, justinruggles.
xur added subscribers: cfe-commits, xur, mcrosier and 2 others.
silvas edited edge metadata.Feb 25 2016, 9:52 PM

Thanks for splitting this out.

I think the -fprofile-instrument=llvm is straightforward and uncontroversial. Let's focus this review on that and split the others into a separate review.

But a quick preview of my thoughs about -fprofile-instrument={llvm,clang}-use:
I think that fully decoupling the cc1 options from the driver options like you are doing here is a good cleanup, but we need a more refined approach. For example, the current patch has "use" modes as mutually exclusive with the "gen" modes, which doesn't make sense. In fact, we have already seen a situation where we may want to have "use" and "gen" in the same clang invocation: for profile guided MST instr. And I don't see a reason to artificially prohibit FE "use" to annotate MD_prof metadata and then use that metadata for profile guided MST instr (however, it would be a strange workflow for users...).
Perhaps something like -fprofile-instrument-use={llvm,clang} which sets a CodeGenOpt ProfileUse which is either {None, Clang, LLVM}. We can use a common PGOInstrumentor enum for both, since there is a 1:1 mapping between the instrumentor and its corresponding "use" logic.
In the long run I imagine we will want two mirror options -fpgo-gen={none,llvm,clang} and -fpgo-use={none,llvm,clang}, but the details of renaming to the final state can wait until we clearly see the right design (this is cc1 so it is easy to refactor as needed later).

xur updated this revision to Diff 49243.Feb 26 2016, 2:31 PM
xur edited edge metadata.

Sean, Thanks for the review and suggestions.

Here is the patch that only adds -fprofile-instrument=llvm.

I'll have another patch using your suggestion on cc1 options for profile use compilation.

silvas accepted this revision.Feb 26 2016, 8:53 PM
silvas edited edge metadata.

Thanks. LGTM.

This revision is now accepted and ready to land.Feb 26 2016, 8:53 PM
justinruggles resigned from this revision.Feb 27 2016, 11:40 AM
justinruggles removed a reviewer: justinruggles.
This revision was automatically updated to reflect the committed changes.