[PGO] Clang Option that enables IR level PGO instrumentation

[xur]

Summary

This patch introduce a new toggle option -fprofile-ir-instr that enables IR level instrumentation. It needs to be used in combination with current PGO options. Without an existing PGO options, this option alone is a null operation.

This patch depends on llvm patch http://reviews.llvm.org/D15828 where we introduce a pair of passmanagerbuilder options:
-profile-generate=<profile_filename>
-profile-use=<profile_filename>
Note that the profile specified in driver level options (-fprofile-instr-generate, -fprofile-instr-use, -fprofile-generate, and -profile-use) overrides the profile in the passmanagerbuilder option.

[davidxl]

Should add a test case in test/Driver/instrprof-ld.c.

lib/CodeGen/BackendUtil.cpp

440

What is this asserting about?

lib/Frontend/CompilerInvocation.cpp

481

Add a comment here.

[xur]

I'll sent the revised patch based on the comments shortly.

[xur]

lib/CodeGen/BackendUtil.cpp

440

Option ProfileInstrGenerate is to be used in Clang instrumentation only. If I reuse this options, I would need to change all the references in clang. I update the comments in the coming patch.

[xur]

added new tests and comments based on the review comments.

[xur]

Ping.

Passmanagerbuilder change has been committed. Could you take a look at this command line option patch?

Thanks,

-Rong

[silvas]

This needs tests showing that the IR gen/use passes get run. Maybe use -debug-pass=Structure like test/CodeGen/thinlto_backend.c?

My biggest concern is the naming and user visible parts. I can't come up with anything better than -fprofile-ir-instr TBH. Overall, from a user's perspective, this is really just "perform instrumentation in an alternate way", since "IR" vs "frontend" is really not meaningful for them (there isn't really any meaningful way for use to communicate anything to them about what they should expect the flag to do differently as user-visible behavior). -fprofile-alternate-instr doesn't sound any better really.

Any ideas?

Once we expose this as a driver option though we must remain compatible, so it is best to think for a moment about the naming.

include/clang/Driver/Options.td

457 ↗

(On Diff #44776)

This doesn't seem like useful help text for a user.

[silvas]

include/clang/Frontend/CodeGenOptions.def

107

tiny nit: the word "code" seems redundant here.

[silvas]

@slingn and I had a discussion offline about the potential names and came up with some ideas, but none is a clear winner.

Overall, my feeling is that from a user's perspective, the frontend stuff is probably best referred to as "coverage-based". It's not as clear for the IR-level stuff, but referring to it as the "pgo focused" or "optimization focused" instrumentation might be a way to describe it. E.g. perhaps -fprofile-instr-method={coverage,optimization}.

For now, can we make this a CC1-only option? Then we don't have to hold up the patch review on the driver stuff. Once we have fully integrated the IR-level instrumentation, we can revisit exposing the user-visible name. (as compiler developers, we can of course use the flag freely for integration/testing).

[silvas]

@bogner btw, did you say that at Apple you guys have a requirement of supporting existing profdata? I.e. users can pass older profdata to a newer compiler?

Realistically, it would be nice if our PGO offering defaulted to the IR stuff (since it seems like it is going to be the focus of optimization work and so is likely to be our best-performing offering). But if we need to support profdata across versions of clang, that puts us in a thorny situation because suddenly clang -c foo.c -fprofile-instr-use=old-profdata-from-frontend-instrumentation.profdata has a default meaning equivalent to clang -c foo.c -fir-level-instrumentation -fprofile-instr-use=old-profdata-from-frontend-instrumentation.profdata (setting aside the flag name issue), which is a case we want to error on due to mismatch between frontend and IR-level instrumentation.

[davidxl]

This option is not needed for -fprofile-instr-use compilation as the compiler can detect the flavor of the profile (patch pending review), and decide what to do.

The question here is what the option should look like to tell the compiler which phase to do the instrumentation (fprofile-instr-generate).

[xur]

This new patches integrates Sean review comments:
(1) make -fprofile-ir-instr a cc1 option instead of driver option.
(2) add one cc1 option test, and test the pass is indeed invoked.
(3) remove the driver test (runtime library).
(4) fix a comment.

Thanks,

-Rong

[mcrosier]

Would it make sense to include an additional test (in test/Driver) that shows the -fprofile-ir-instr option being passed from the driver to the frontend? Such a test case would land in clang_f_opt.c, which has many examples.

lib/Driver/Tools.cpp

3279 ↗

(On Diff #45708)

I don't think AddAllArgs is what you really want. What if the user specifies the option twice? Do we really want to pass the flag from the driver to the front-end twice? Also, should we warn if the option is passed twice?

I also think some of the logic in CompilerInvocation should land here... see comments below.

lib/Frontend/CompilerInvocation.cpp

482

IIRC, the general practice is to put this type of logic in the driver. Specifically, in Driver.cpp include something like

if (Args.hasArg(OPT_fprofile_ir_instr) &&

  (Args.hasArg(OPT_fprofile_instr_generate) ||
   Args.hasArg(OPT_fprofile_instr_generate_EQ) ||
   Args.hasArg(OPT_fprofile_instr_use_EQ)))
// Add -fprofile_ir_instr flag....

[xur]

lib/Driver/Tools.cpp

3279 ↗

(On Diff #45708)

Thanks for pointing thi out. What about add a guard, like:

• Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);

+ if (Args.hasArg(options::OPT_fprofile_ir_instr))
+ Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);

But looking at it again, I think i need to remove this stmt in the most recently patch as OPT_profile_ir_instr is now a CC1 option. it will not be seen here, right?

lib/Frontend/CompilerInvocation.cpp

482

In the most recent patch (Suggested by Sean as a temporary measure to speed up the code review) where -fprofiel-ir-instr becomes a CC1 only option, I have to do it here. Driver won't see the OPT_fprofile_ir_instr.

But I'll remember you point when this becomes a Driver option in the next step. Also note that the driver could convert "-fprofile-genearte" to "-fprofiel-instr-generate" and also for "-fprofile-use". So the condition will be longer if we move to driver.

[davidxl]

For the longer term, one possible solution is to make FE based
instrumentation only used for coverage testing which can be turned on
with -fcoverage-mapping option (currently, -fcoverage-mapping can not
be used alone and must be used together with
-fprofile-instr-generate). To summarize:

A. Current behavior:

1. -fprofile-instr-generate turns on FE based instrumentation
2. -fprofile-instr-generate -fcoverage-mapping turns on FE based

instrumentation and coverage mapping data generation.

3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.

B. Proposed new behavior:

1. -fprofile-instr-generate turns on IR late instrumentation
2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
4. -fprofile-instr-use=<> will automatically determine how to use the

profile data.

B.2) above can be done today for improved usability. B.1) needs a
transition period before the IR based instrumentation becomes
stablized (and can be flipped to the default). During the transition
period, the behavior of 1) does not change, but a cc1 option can be
used to turn on IR instrumentation (as proposed by Sean).

In the real longer term, I think IR based instrumentation can also be
used for coverage testing too (by disabling some pre-optimizations and
pre-inlining needed for PGO purpose) -- but this is a different topic
to be discussed.

thanks,

David

[silvas]

lib/Driver/Tools.cpp

3279 ↗

(On Diff #45708)

But looking at it again, I think i need to remove this stmt in the most recently patch as OPT_profile_ir_instr is now a CC1 option. it will not be seen here, right?

Yes. The patch should make no changes to lib/Driver and should require no tests in test/Driver

[mcrosier]

lib/Driver/Tools.cpp

3279 ↗

(On Diff #45708)

Ah, okay. I still think this type of complexity should land in the driver, but if this is a special case then just ignore my comments.

[silvas]

In D15829#333902, @davidxl wrote:

For the longer term, one possible solution is to make FE based
instrumentation only used for coverage testing which can be turned on
with -fcoverage-mapping option (currently, -fcoverage-mapping can not
be used alone and must be used together with
-fprofile-instr-generate). To summarize:

A. Current behavior:

1. -fprofile-instr-generate turns on FE based instrumentation
2. -fprofile-instr-generate -fcoverage-mapping turns on FE based

instrumentation and coverage mapping data generation.

3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation.

B. Proposed new behavior:

1. -fprofile-instr-generate turns on IR late instrumentation
2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping
3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning
4. -fprofile-instr-use=<> will automatically determine how to use the

profile data.

Very good observation that we can determine FE or IR automatically based on the input profdata. That simplifies things.

B.2) above can be done today for improved usability.

I don't see how this improves usability. In fact, it is confusing because it hijacks an existing option.

Also B.3 causes existing user builds to emit a warning, which is annoying.

I would propose the following modification of B:

C.:

1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves exactly as before, except that it uses IR instrumentation)
2. -fprofile-instr-generate -fcoverage-mapping turns on frontend instrumentation and coverage. (i.e. behaves exactly as before)
3. -fprofile-instr-use=<> automatically determines which method to use

All existing user workflows continue to work, except for workflows that attempt to llvm-profdata merge some old frontend profile data (e.g. they have checked-in to version control and represents some special workload) with the profile data from new binaries.

Concretely, imagine the following workflow:

clang -fprofile-instr-generate foo.c -o foo
./foo
llvm-profdata merge default.profraw -o new.profdata
llvm-profdata merge new.profdata /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata -o foo.profdata
clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo

I think this is a reasonable breakage. We would need to add a note in the release notes. Unfortunately this is not expected breakage if we claim to have forward compatibility for profdata (which IIRC Apple requires; @bogner?). But I think this case will be rare and exceptional enough that we can tolerate it:

• a simple immediate workaround is to specify -fcoverage-mapping (which also adds some extra stuff, but works around the issue)
• Presumably /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata is regenerated with some frequency which is more frequent than upgrading the compiler, and so it is likely reasonable to regenerate it alongside a compiler upgrade, using the workaround until then.

B.1) needs a
transition period before the IR based instrumentation becomes
stablized (and can be flipped to the default). During the transition
period, the behavior of 1) does not change, but a cc1 option can be
used to turn on IR instrumentation (as proposed by Sean).

Just to clarify, users are not allowed to use cc1 options. The cc1 option is purely for us as compiler developers to do integration and testing, put off some discussions for later, etc.

[silvas]

lib/CodeGen/BackendUtil.cpp

440

Then change the existing references in clang. Please try to cleanly integrate the new functionality, not "sneak it in" with the smallest number of lines changed (and creating technical debt for future maintainers).

lib/Frontend/CompilerInvocation.cpp

482

Chad makes a good point though. This should be just Opts.ProfileIRInstr = Args.hasArg(OPT_fprofile_ir_instr); because we can assume that for cc11 this option is not used except in conjunction with -fprofile-instr-{generate,use}

487

I don't like this. It breaks the 1:1 mapping of flags to codegen options. Like Chad said, this sort of complexity should be kept in the driver.

Some refactoring may be needed to cleanly integrate the new IR instrumentation.

test/CodeGen/pgo-instrumentation.c

10

Please test both Gen and Use phases.

Also, please use more descriptive CHECK names.

[xur]

lib/CodeGen/BackendUtil.cpp

440

The integration is actually clean to me: For CodeGenOpt: ProfileInstrGenerate is for Clang instrumentation and ProfileIRInstr is for IR instrumentation. They need to mutually exclusive.

Maybe I need to modify the name and description for ProfileInstrGenerate to reflect the change.

lib/Frontend/CompilerInvocation.cpp

482

If we can have the assumption. Yes I can simplify the stmt. If the user breaks the assumption. Current patch will not invoke the IR PGO pass, while the proposed way will cause error. Of course, you can say this is a usage error.

I have no problem to simplify this.

487

hmm. I don't think there is 1:1 mapping b/w flags and codegen options, because -fprofile-instr-generate is shared by IR and FE instrumentation.

[davidxl]

lib/CodeGen/BackendUtil.cpp

440

yes, you can change ProfileInstrGenerate to something like ClangProfInstrGen as a NFC first.

lib/Frontend/CompilerInvocation.cpp

487

If you rename the FE PGO CodeGen opt name as proposed, it will be clearer and less confusing.

[silvas]

lib/Frontend/CompilerInvocation.cpp

487

hmm. I don't think there is 1:1 mapping b/w flags and codegen options, because -fprofile-instr-generate is shared by IR and FE instrumentation.

Maybe at the driver level (that discussion has yet to conclude), but at cc1 level options should be factored to be clean and orthogonal. Like Chad said, the complexity of verifying options that are mutually exclusive or that must be present with other options should be done in the driver. cc1 is a private interface

[xur]

OK. Now I understand what you meant. I was trying to avoid driver changes.
It seems you are saying I can change the driver to support cc1 options more
cleanly. This can be done.

Thanks,

Rong

[xur]

Here is the new patch. Changes from previous patch are:
(1) rename codegen option ProfileInstrGenerate to ClangProfInstrGen to avoid the name confusion (Davidxl). This option is for clang instrumentation only.
(2) improve the test suggested by Sean.
(3) move the logic of checking if argument ProfileIRInstr needs to be added to Tools.C (suggested by Chad and Sean). This still does not look very clean -- I need to manually expand the -Xclang option for this. The difficulty here is that we have shared driver level arguments (-fprofile-generate and -fprofile-use etc) and there is no cc1 option for clang instrumentation. Once we have driver level option for IR level instrumentation, the code can be improved.

[silvas]

lib/Driver/Tools.cpp

5520 ↗

(On Diff #45929)

This is definitely not the right thing to do. Don't hijack -Xclang (which is a completely generic thing).

lib/Frontend/CompilerInvocation.cpp

483

This still isn't factored right. I think at this point the meaning of the driver-level options is not really useful at CC1 level (too convoluted) for it to be useful to pass them through.

The right thing to do here is probably more like:

• refactor so that we have 4 individual CC1 options for InstrProfileOutput, InstrProfileInput, ProfileIRInstr, ClangProfInstrGen (although probably rename ClangProfInstrGen and ProfileIRInstr to be consistent with each other, e.g. "ProfileIRInstr" and "ProfileClangInstr").
• add logic in Driver to convert from the driver-level options to the CC1 options.

test/CodeGen/pgo-instrumentation.c

6

It isn't clear what V1/V2 mean.

[xur]

Sean: Adding a new CC1 option ProfileClangInstr will make things
cleaner. But this won't solve the problem. The root of all the mess is
there is no driver level option for IR instrumentation. I need to
either "hijack" the -Xclang option, or move the logic to
CompilerInvocation.cpp, which both you don't like.

The reason is I have to reply on the Driver option
-fprofile-instr-generate to have the right link line for the profile
library. -fprofile-instr-generate will set the Instrumentation to
Clang (regardless use of current cc1 option of
-fprofile-instr-generate, or the new proposed -fprofile-clang-instr).
For IR instrumentation where the user specifies "-Xclang
-fprofile-ir-instr", I need to overwrite the driver level option. To
get that, I either parse the -Xclang value (this is "hijack), or I
handle it in CC1 (in CompilerInvocation.cpp). I don't see a way to
avoid it.

Can we use a hidden driver option here for IR instrumentation?

[davidxl]

lib/Driver/Tools.cpp

5520 ↗

(On Diff #45929)

Why do we need special handling here ? will the existing behavior (simple forwarding) work?

Note that intention of the cc1 option is to hide it from the user but make it used by the developers -- so we can make assumption that this option is used only when -fprofile-instr-generate[=...] is specified. You can add diagnostics later during cc1 option processing if it is not the case.

Also think about making it easier to flip the default behavior in the future, it might be better to make the cc1 option look like this:

-fprofile-instrumentor=[clang|LLVM]

if the option is missing, the current default will be 'clang'. In the future just switch it to 'llvm'.

lib/Frontend/CompilerInvocation.cpp

483

It is already pretty close to your proposal -- the only missing thing is cc1 option for ClangProfInstrGen. However given that IR and Clang InstrGen are mutually exclusive, is an additional CC1 option needed?

[xur]

This new version implemented the usage model David proposed. We now have a cc1 option -fprofile-instrumentor={llvm | clang} to choose which instrumentation to use. The slight difference from David's proposal is that we can use this option with -fprofile-instr-use.

Thanks,

-Rong

[davidxl]

include/clang/Frontend/CodeGenOptions.def

106

For the sake of being consistent with the new cc1 option, making this an enum option is better:

ENUM_CODEGENOPT(ProfileInstr,... ) which takes two legal values ProfInstrClang, ProfInstrLLVM

I'd like to hear Sean's opinion on this.

[silvas]

lib/Frontend/CompilerInvocation.cpp

483

There are 3 possibilities:

• clang instr
• ir instr
• no instr

A simple binary flag can only encode 2. So some sort of extra information needs to be passed. Off the top of my head, I thought of just adding an extra flag, but I like your suggestion -fprofile-instrumentor=[clang|LLVM]. That is clean and avoids having to deal with an error for the "clang instr + ir instr" case.

[silvas]

include/clang/Frontend/CodeGenOptions.def

106

SGTM. I was going to suggest something similar. This is more consistent with e.g. OPT_debug_info_kind_EQ and the other multi-value options.

lib/CodeGen/CodeGenModule.cpp

150

This seems like a latent bug: !CodeGenOpts.ProfileIRInstr is true when we are doing no instrumentation. Using an ENUM_CODEGENOPT will fix this and related issues.

lib/Frontend/CompilerInvocation.cpp

495

The work being done in this else is redundant. -fprofile-instrumentor={clang,llvm,none} (with "none" being the default if the argument isn't passed) covers this case as -fprofile-instrumentor=clang. The driver can be changed to translate -fprofile-instr-generate into -fprofile-instrumentor=clang. (and -fprofile-instrument= might be a better name)

[xur]

I'll send an updated patch shortly.

include/clang/Frontend/CodeGenOptions.def

106

OK. I'll use ENUM as you two suggested.

lib/CodeGen/CodeGenModule.cpp

150

I don't think this is a bug. As I mentioned in the update comments. I still use this option (-fprofile-instrumentor=) in profile-use compilation. Once we have the llvm-profdata patch in, we can detect the profile kind and remove this.

lib/Frontend/CompilerInvocation.cpp

495

Only if we changed the driver and pushed in -fprofile-instrumentor to the cc1 argument list, the else part is indeed redundant.

But since I did not change the driver options handling, we still need this else branch -- we only set ProfileClangInstr when there is OPT_fprofile_instr_generate.

I'll change the -fprofile-instrumentor to -fprofile-instrument and change the driver to push in this argument. I will probably remove OPT_fprfoile_instr_generate as a CC1 option.

[xur]

Integrated most recently comments/suggestions from David and Sean.

Thanks,

-Rong

[xur]

The previous patch was not good as it failed 58 tests that use
-fprofile-instr-generate as a cc1 option.

I can see two methods to solve this:
(1) we change all the 58 tests to use -fprofile-instrument=Clang option.
(2) Fall back to my previous patch: keep -fprofile-instr-generate as
the CC1 options and check it in Frontend/CompilerInvocation.cpp. (i.e.
The redundant else branch Sean was referring to). We have to do it
here because we cannot assume driver will append
-fprofile-instrument=Clang in the case the user uses
-fprofile-instr-generate as the cc1 option.

[davidxl]

lib/Driver/Tools.cpp

3232 ↗

(On Diff #46303)

I also suggest changing CC1 option -fprofile-instr-generate= to be a different name (from the driver option): -fprofile-instrument-path=... or something.

[xur]

This new patch adds the change the clang test cases (cc1 option -fprofile-instr-generate to -fprofile-instrument=Clang).

It also replaces cc1 option -fprofile-instr-generate= to -fprofile-instrument-path=, as suggested by David.

Thanks,

-Rong

[xur]

To make the review easier, I split the cc1 names change part into a new NFC patch here.
http://reviews.llvm.org/D16730

[xur]

Now http://reviews.llvm.org/D16730 has been committed (as r259811)

Here is the patch that adds cc1 option -fprofile-instrument=llvm to enable IR level PGO generate and use.

The pgo use part of the patch depends http://reviews.llvm.org/D15540 which differentiates the IR level and clang profiles. To detect the profile automatically, we need to first the profile. It can either done in driver or in Clang codegen (lib/CodeGen/CodeGenModule.cpp). If we do this in Clang codegen, we would have to change the Codegen option which does not seem to be a good approach. As a result, we do this in the driver in this patch.

Again like in D16730, this patch won't change any driver (user level) options. Only cc1 options are touched.

Thanks,

-Rong

[davidxl]

include/clang/Frontend/CodeGenOptions.h

234 ↗

(On Diff #46960)

typo: instrumentation

[xur]

fixed the typo in comments

[xur]

Ping

(also added the test that missed from last upload).

Thanks,

-Rong

[davidxl]

lib/CodeGen/CodeGenModule.cpp

150

Better to use if (CodeGenOpts.hasProfileClangInstr() && ..)

lib/Driver/Tools.cpp

3201 ↗

(On Diff #47331)

I don't quite like this change in the driver. I think the right thing to do is:

1. for profile use case, there is no need to pass -fprofile-instrument=<...> FE option from the driver
2. The profileInstrKind determination needs to happen in FE (not driver) by peeking into the passed in profile data.

[silvas]

Apologies for the delay. At this point, I think that this patch has evolved enough that it is best to start a new patch. I think the steps forward are:

• Have cc1 accept -fprofile-instrument=llvm (requires no driver changes, but is enough for developers to test by being careful and manually passing it)
• Start a discussion on llvm-dev to decide on the driver-level
• implement other driver features as discussed
  • e.g. if we decide that -fprofile-instr-use= should detect FE/IR (i.e. IR is not a separate flag), add FE/IR detection for -fprofile-instr-use=

test/CodeGen/pgo-instrumentation.c

5

Use %clang_cc1 here and elsewhere.

[xur]

I'll send a revised patch soon.

lib/CodeGen/CodeGenModule.cpp

150

OK. That would require to insert an extra cc1 option of -profile-instrument=clang for profile-use. And possibly some test case option changes too.

lib/Driver/Tools.cpp

3201 ↗

(On Diff #47331)

I'll split off the profile kind detection to a separated patch suggested by Sean.

test/CodeGen/pgo-instrumentation.c

5

Will do.

[xur]

Here is the new patch that removes the auto detection of profile kind.

In this patch, I replace the CC1 option of -fprofile-instr-use=<> with -fprofile-instrument={llvm-use|clang-use}. For the use compilation, the profile reuses the -fprofile-instrument-path= option.

Again, all changes are of CC1 options. Driver options are intact.

Some test 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

[davidxl]

include/clang/Frontend/CodeGenOptions.h

83 ↗

(On Diff #48846)

Maybe ProfileNone, // Profile instrumentation and use is turned off.

lib/CodeGen/CodeGenModule.cpp

151

Is the second condition still needed? Should it be asserted ?

test/CodeGen/pgo-instrumentation.c

5

should be %clang_cc1

9

%clang_cc1

[xur]

lib/CodeGen/CodeGenModule.cpp

151

The second condition is not needed. I'll change the code in the coming patch.

test/CodeGen/pgo-instrumentation.c

5

I think both %clang_cc1 and %clang-cc1 are accepted. But all the other tests are using %clang_cc1. I'll change.

[xur]

Integrated with David's comments.

Thanks,

-Rong

[davidxl]

Looks good to me -- and it makes the profile-gen and profile-use's cc1 option handling consistent. Please check with Sean or Justin just in case before proceeding.

[silvas]

In D15829#360006, @xur wrote:

Here is the new patch that removes the auto detection of profile kind.

In this patch, I replace the CC1 option of -fprofile-instr-use=<> with -fprofile-instrument={llvm-use|clang-use}. For the use compilation, the profile reuses the -fprofile-instrument-path= option.

Again, all changes are of CC1 options. Driver options are intact.

Some test 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

I meant in a new phabricator review. This one is has gone on for too long and is convoluted and hard to follow. (lots of stray inline comments etc.)

[xur]

Create a new review here:
http://reviews.llvm.org/D17622

Thanks,

-Rong

include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_unknown_language : Error<"language not recognized: '%0'">;
 def err_drv_invalid_arch_name : Error<
   "invalid arch name '%0'">;
 def err_drv_cuda_bad_gpu_arch : Error<"Unsupported CUDA gpu architecture: %0">;
 def err_drv_invalid_thread_model_for_target : Error<
   "invalid thread model '%0' in '%1' for this target">;
 def err_drv_invalid_linker_name : Error<
   "invalid linker name in argument '%0'">;
+def err_drv_invalid_pgo_instrumentor: Error<
+  "invalid PGO instrumentor in argument '%0'">;
 def err_drv_invalid_rtlib_name : Error<
   "invalid runtime library name in argument '%0'">;
 def err_drv_unsupported_rtlib_for_platform : Error<
   "unsupported runtime library '%0' for platform '%1'">;
 def err_drv_invalid_stdlib_name : Error<
   "invalid library name in argument '%0'">;
 def err_drv_invalid_output_with_multiple_archs : Error<
   "cannot use '%0' output with multiple -arch options">;

include/clang/Driver/CC1Options.td

def fsanitize_coverage_trace_bb
     : Flag<["-"], "fsanitize-coverage-trace-bb">,
       HelpText<"Enable basic block tracing in sanitizer coverage">;
 def fsanitize_coverage_trace_cmp
     : Flag<["-"], "fsanitize-coverage-trace-cmp">,
       HelpText<"Enable cmp instruction tracing in sanitizer coverage">;
 def fsanitize_coverage_8bit_counters
     : Flag<["-"], "fsanitize-coverage-8bit-counters">,
       HelpText<"Enable frequency counters in sanitizer coverage">;
+def fprofile_instrumentor_EQ: Joined<["-"], "fprofile-instrumentor=">,
+    HelpText<"Specify the instrumentor that instruments the code to generate "
+             "execution counts to use with PGO">;
 
 //===----------------------------------------------------------------------===//
 // Dependency Output Options
 //===----------------------------------------------------------------------===//
 
 def sys_header_deps : Flag<["-"], "sys-header-deps">,
   HelpText<"Include system headers in dependency output">;
 def module_file_deps : Flag<["-"], "module-file-deps">,

include/clang/Frontend/CodeGenOptions.def

 CODEGENOPT(NoZeroInitializedInBSS , 1, 0) ///< -fno-zero-initialized-in-bss.
 /// \brief Method of Objective-C dispatch to use.
 ENUM_CODEGENOPT(ObjCDispatchMethod, ObjCDispatchMethodKind, 2, Legacy) 
 CODEGENOPT(OmitLeafFramePointer , 1, 0) ///< Set when -momit-leaf-frame-pointer is
                                         ///< enabled.
 VALUE_CODEGENOPT(OptimizationLevel, 2, 0) ///< The -O[0-3] option specified.
 VALUE_CODEGENOPT(OptimizeSize, 2, 0) ///< If -Os (==1) or -Oz (==2) is specified.
 
-CODEGENOPT(ProfileInstrGenerate , 1, 0) ///< Instrument code to generate
-                                        ///< execution counts to use with PGO.
+CODEGENOPT(ProfileClangInstr, 1, 0) ///< Clang Instrumentation to generate
+CODEGENOPT(ProfileIRInstr, 1, 0) ///< IR level PGO instrumentation and use.
 CODEGENOPT(CoverageMapping , 1, 0) ///< Generate coverage mapping regions to
                                    ///< enable code coverage analysis.
 CODEGENOPT(DumpCoverageMapping , 1, 0) ///< Dump the generated coverage mapping
                                        ///< regions.
 
   /// If -fpcc-struct-return or -freg-struct-return is specified.
 ENUM_CODEGENOPT(StructReturnConvention, StructReturnConventionKind, 2, SRCK_Default)
 

lib/CodeGen/BackendUtil.cpp

if (!CodeGenOpts.DisableGCov &&
     Options.FunctionNamesInData =
         !CodeGenOpts.CoverageNoFunctionNamesInData;
     Options.ExitBlockBeforeBody = CodeGenOpts.CoverageExitBlockBeforeBody;
     MPM->add(createGCOVProfilerPass(Options));
     if (CodeGenOpts.getDebugInfo() == CodeGenOptions::NoDebugInfo)
       MPM->add(createStripSymbolsPass(true));
   }
 
-  if (CodeGenOpts.ProfileInstrGenerate) {
+  if (CodeGenOpts.ProfileClangInstr) {
     InstrProfOptions Options;
     Options.NoRedZone = CodeGenOpts.DisableRedZone;
     Options.InstrProfileOutput = CodeGenOpts.InstrProfileOutput;
     MPM->add(createInstrProfilingPass(Options));
   }
 
+  if (CodeGenOpts.ProfileIRInstr) {
+    if (!CodeGenOpts.InstrProfileInput.empty())
+      PMBuilder.PGOInstrUse = CodeGenOpts.InstrProfileInput;
+    else if (!CodeGenOpts.InstrProfileOutput.empty())
+      PMBuilder.PGOInstrGen = CodeGenOpts.InstrProfileOutput;
+    else
+      PMBuilder.PGOInstrGen = "default.profraw";
+  }
+
   if (!CodeGenOpts.SampleProfileFile.empty())
     MPM->add(createSampleProfileLoaderPass(CodeGenOpts.SampleProfileFile));
 
   PMBuilder.populateModulePassManager(*MPM);
 }
 
 TargetMachine *EmitAssemblyHelper::CreateTargetMachine(bool MustCreateTM) {
   // Create the TargetMachine for generating code.

lib/CodeGen/CGStmt.cpp

void CodeGenFunction::EmitCaseStmt(const CaseStmt &S) {
   }
 
   llvm::ConstantInt *CaseVal =
     Builder.getInt(S.getLHS()->EvaluateKnownConstInt(getContext()));
 
   // If the body of the case is just a 'break', try to not emit an empty block.
   // If we're profiling or we're not optimizing, leave the block in for better
   // debug and coverage analysis.
-  if (!CGM.getCodeGenOpts().ProfileInstrGenerate &&
+  if (!CGM.getCodeGenOpts().ProfileClangInstr &&
       CGM.getCodeGenOpts().OptimizationLevel > 0 &&
       isa<BreakStmt>(S.getSubStmt())) {
     JumpDest Block = BreakContinueStack.back().BreakBlock;
 
     // Only do this optimization if there are no cleanups that need emitting.
     if (isObviouslyBranchWithoutCleanups(Block)) {
       if (SwitchWeights)
         SwitchWeights->push_back(getProfileCount(&S));
void CodeGenFunction::EmitCaseStmt(const CaseStmt &S) {
   // Otherwise, iteratively add consecutive cases to this switch stmt.
   while (NextCase && NextCase->getRHS() == nullptr) {
     CurCase = NextCase;
     llvm::ConstantInt *CaseVal =
       Builder.getInt(CurCase->getLHS()->EvaluateKnownConstInt(getContext()));
 
     if (SwitchWeights)
       SwitchWeights->push_back(getProfileCount(NextCase));
-    if (CGM.getCodeGenOpts().ProfileInstrGenerate) {
+    if (CGM.getCodeGenOpts().ProfileClangInstr) {
       CaseDest = createBasicBlock("sw.bb");
       EmitBlockWithFallThrough(CaseDest, &S);
     }
 
     SwitchInsn->addCase(CaseVal, CaseDest);
     NextCase = dyn_cast<CaseStmt>(CurCase->getSubStmt());
   }
 

lib/CodeGen/CodeGenFunction.h

private:
   llvm::MDNode *createProfileWeights(uint64_t TrueCount, uint64_t FalseCount);
   llvm::MDNode *createProfileWeights(ArrayRef<uint64_t> Weights);
   llvm::MDNode *createProfileWeightsForLoop(const Stmt *Cond,
                                             uint64_t LoopCount);
 
 public:
   /// Increment the profiler's counter for the given statement.
   void incrementProfileCounter(const Stmt *S) {
-    if (CGM.getCodeGenOpts().ProfileInstrGenerate)
+    if (CGM.getCodeGenOpts().ProfileClangInstr)
       PGO.emitCounterIncrement(Builder, S);
     PGO.setCurrentStmt(S);
   }
 
   /// Get the profiler's count for the given statement.
   uint64_t getProfileCount(const Stmt *S) {
     Optional<uint64_t> Count = PGO.getStmtCount(S);
     if (!Count.hasValue())

lib/CodeGen/CodeGenFunction.cpp

 
 /// When instrumenting to collect profile data, the counts for some blocks
 /// such as switch cases need to not include the fall-through counts, so
 /// emit a branch around the instrumentation code. When not instrumenting,
 /// this just calls EmitBlock().
 void CodeGenFunction::EmitBlockWithFallThrough(llvm::BasicBlock *BB,
                                                const Stmt *S) {
   llvm::BasicBlock *SkipCountBB = nullptr;
-  if (HaveInsertPoint() && CGM.getCodeGenOpts().ProfileInstrGenerate) {
+  if (HaveInsertPoint() && CGM.getCodeGenOpts().ProfileClangInstr) {
     // When instrumenting for profiling, the fallthrough to certain
     // statements needs to skip over the instrumentation code so that we
     // get an accurate count.
     SkipCountBB = createBasicBlock("skipcount");
     EmitBranch(SkipCountBB);
   }
   EmitBlock(BB);
   uint64_t CurrentCount = getCurrentProfileCount();

lib/CodeGen/CodeGenModule.cpp

if (CodeGenOpts.getDebugInfo() != CodeGenOptions::NoDebugInfo ||
       CodeGenOpts.EmitGcovNotes)
     DebugInfo = new CGDebugInfo(*this);
 
   Block.GlobalUniqueCount = 0;
 
   if (C.getLangOpts().ObjC1)
     ObjCData = new ObjCEntrypoints();
 
-  if (!CodeGenOpts.InstrProfileInput.empty()) {
+  if (!CodeGenOpts.ProfileIRInstr && !CodeGenOpts.InstrProfileInput.empty()) {
     auto ReaderOrErr =
         llvm::IndexedInstrProfReader::create(CodeGenOpts.InstrProfileInput);
     if (std::error_code EC = ReaderOrErr.getError()) {
       unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
                                               "Could not read profile %0: %1");
       getDiags().Report(DiagID) << CodeGenOpts.InstrProfileInput
                                 << EC.message();
     } else
       PGOReader = std::move(ReaderOrErr.get());

lib/CodeGen/CodeGenPGO.cpp

 void CodeGenPGO::setFuncName(StringRef Name,
                              llvm::GlobalValue::LinkageTypes Linkage) {
   llvm::IndexedInstrProfReader *PGOReader = CGM.getPGOReader();
   FuncName = llvm::getPGOFuncName(
       Name, Linkage, CGM.getCodeGenOpts().MainFileName,
       PGOReader ? PGOReader->getVersion() : llvm::IndexedInstrProf::Version);
 
   // If we're generating a profile, create a variable for the name.
-  if (CGM.getCodeGenOpts().ProfileInstrGenerate)
+  if (CGM.getCodeGenOpts().ProfileClangInstr)
     FuncNameVar = llvm::createPGOFuncNameVar(CGM.getModule(), Linkage, FuncName);
 }
 
 void CodeGenPGO::setFuncName(llvm::Function *Fn) {
   setFuncName(Fn->getName(), Fn->getLinkage());
 }
 
 namespace {
uint64_t PGOHash::finalize() {
   llvm::MD5::MD5Result Result;
   MD5.final(Result);
   using namespace llvm::support;
   return endian::read<uint64_t, little, unaligned>(Result);
 }
 
 void CodeGenPGO::assignRegionCounters(GlobalDecl GD, llvm::Function *Fn) {
   const Decl *D = GD.getDecl();
-  bool InstrumentRegions = CGM.getCodeGenOpts().ProfileInstrGenerate;
+  bool InstrumentRegions = CGM.getCodeGenOpts().ProfileClangInstr;
   llvm::IndexedInstrProfReader *PGOReader = CGM.getPGOReader();
   if (!InstrumentRegions && !PGOReader)
     return;
   if (D->isImplicit())
     return;
   // Constructors and destructors may be represented by several functions in IR.
   // If so, instrument only base variant, others are implemented by delegation
   // to the base one, it would be counted twice otherwise.
CodeGenPGO::applyFunctionAttributes(llvm::IndexedInstrProfReader *PGOReader,
   if (!haveRegionCounts())
     return;
 
   uint64_t FunctionCount = getRegionCount(nullptr);
   Fn->setEntryCount(FunctionCount);
 }
 
 void CodeGenPGO::emitCounterIncrement(CGBuilderTy &Builder, const Stmt *S) {
-  if (!CGM.getCodeGenOpts().ProfileInstrGenerate || !RegionCounterMap)
+  if (!CGM.getCodeGenOpts().ProfileClangInstr || !RegionCounterMap)
     return;
   if (!Builder.GetInsertBlock())
     return;
 
   unsigned Counter = (*RegionCounterMap)[S];
   auto *I8PtrTy = llvm::Type::getInt8PtrTy(CGM.getLLVMContext());
   Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::instrprof_increment),
                      {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
                       Builder.getInt64(FunctionHash),
                       Builder.getInt32(NumRegionCounters),
                       Builder.getInt32(Counter)});
 }
 
 // This method either inserts a call to the profile run-time during
 // instrumentation or puts profile data into metadata for PGO use.
 void CodeGenPGO::valueProfile(CGBuilderTy &Builder, uint32_t ValueKind,
     llvm::Instruction *ValueSite, llvm::Value *ValuePtr) {
 
   if (!EnableValueProfiling)
     return;
 
   if (!ValuePtr || !ValueSite || !Builder.GetInsertBlock())
     return;
 
-  bool InstrumentValueSites = CGM.getCodeGenOpts().ProfileInstrGenerate;
+  bool InstrumentValueSites = CGM.getCodeGenOpts().ProfileClangInstr;
   if (InstrumentValueSites && RegionCounterMap) {
     llvm::LLVMContext &Ctx = CGM.getLLVMContext();
     auto *I8PtrTy = llvm::Type::getInt8PtrTy(Ctx);
     llvm::Value *Args[5] = {
         llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
         Builder.getInt64(FunctionHash),
         Builder.CreatePtrToInt(ValuePtr, Builder.getInt64Ty()),
         Builder.getInt32(ValueKind),

lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.UnrollLoops =
       Args.hasFlag(OPT_funroll_loops, OPT_fno_unroll_loops,
                    (Opts.OptimizationLevel > 1 && !Opts.OptimizeSize));
   Opts.RerollLoops = Args.hasArg(OPT_freroll_loops);
 
   Opts.DisableIntegratedAS = Args.hasArg(OPT_fno_integrated_as);
   Opts.Autolink = !Args.hasArg(OPT_fno_autolink);
   Opts.SampleProfileFile = Args.getLastArgValue(OPT_fprofile_sample_use_EQ);
-  Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||
+
+  enum PGOInstrumentor { Unknown, Clang, LLVM };
+  if (Arg *A = Args.getLastArg(OPT_fprofile_instrumentor_EQ)) {
+    StringRef Value = A->getValue();
+    PGOInstrumentor Method = llvm::StringSwitch<PGOInstrumentor>(Value)
+      .Case("clang", Clang)
+      .Case("llvm", LLVM)
+      .Default(Unknown);
+    if (Method == Unknown)
+      Diags.Report(diag::err_drv_invalid_pgo_instrumentor) 
+                     << A->getAsString(Args) << Value;
+    else {
+      Opts.ProfileIRInstr = (Method == LLVM);
+      Opts.ProfileClangInstr = (Method == Clang);
+    }
+  } else {
+    // Default PGO instrumentor is Clang instrumentation.
+    Opts.ProfileClangInstr = Args.hasArg(OPT_fprofile_instr_generate) ||
                              Args.hasArg(OPT_fprofile_instr_generate_EQ);
+    Opts.ProfileIRInstr = false;
+  }
+
   Opts.InstrProfileOutput = Args.getLastArgValue(OPT_fprofile_instr_generate_EQ);
   Opts.InstrProfileInput = Args.getLastArgValue(OPT_fprofile_instr_use_EQ);
   Opts.CoverageMapping =
       Args.hasFlag(OPT_fcoverage_mapping, OPT_fno_coverage_mapping, false);
   Opts.DumpCoverageMapping = Args.hasArg(OPT_dump_coverage_mapping);
   Opts.AsmVerbose = Args.hasArg(OPT_masm_verbose);
   Opts.ObjCAutoRefCountExceptions = Args.hasArg(OPT_fobjc_arc_exceptions);
   Opts.CXAAtExit = !Args.hasArg(OPT_fno_use_cxa_atexit);

test/CodeGen/pgo-instrumentation.c

+// Test if PGO instrumentation and use pass are invoked.
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=llvm -fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationGenPass is invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=llvm -fprofile-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-GEN
+// CHECK-PGOGENPASS-INVOKED-GEN: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationGenPass is not invoked.
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=clang -fprofile-instr-generate %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG
+// CHECK-PGOGENPASS-INVOKED-INSTR-GEN-CLANG-NOT: PGOInstrumentationGenPass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotest.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=llvm -fprofile-instr-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-INSTR-USE
+// CHECK-PGOUSEPASS-INVOKED-INSTR-USE: PGOInstrumentationUsePass
+//
+// Ensure Pass PGOInstrumentationUsePass is invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotest.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=llvm -fprofile-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE
+// CHECK-PGOUSEPASS-INVOKED-USE: PGOInstrumentationUsePass
+//
+// Ensure Pass PGOInstrumentationUsePass is not invoked.
+// RUN: llvm-profdata merge -o %t.profdata %S/Inputs/pgotest.profraw
+// RUN: %clang -O2 -c -Xclang -fprofile-instrumentor=clang -fprofile-use=%t.profdata %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PGOUSEPASS-INVOKED-USE-CLANG
+// CHECK-PGOUSEPASS-INVOKED-USE-CLANG-NOT: PGOInstrumentationUsePass