This is an archive of the discontinued LLVM Phabricator instance.

[PGO] Clang Option that enables IR level PGO instrumentation
Needs ReviewPublic

Authored by xur on Dec 30 2015, 1:51 PM.

Details

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.

Diff Detail

Event Timeline

xur updated this revision to Diff 43806.Dec 30 2015, 1:51 PM
xur retitled this revision from to [PGO] Clang Option that enables IR level PGO instrumentation .
xur updated this object.
xur added reviewers: davidxl, silvas, bogner.
xur added a subscriber: cfe-commits.
davidxl edited edge metadata.Dec 30 2015, 7:51 PM

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

lib/CodeGen/BackendUtil.cpp
453

What is this asserting about?

lib/Frontend/CompilerInvocation.cpp
481

Add a comment here.

slingn added a subscriber: slingn.Jan 4 2016, 9:15 AM
xur marked an inline comment as done.Jan 13 2016, 11:51 AM

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

xur added inline comments.Jan 13 2016, 11:51 AM
lib/CodeGen/BackendUtil.cpp
453

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 updated this revision to Diff 44776.Jan 13 2016, 11:53 AM
xur edited edge metadata.

added new tests and comments based on the review comments.

xur added a comment.Jan 21 2016, 12:59 PM

Ping.

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

Thanks,

-Rong

silvas edited edge metadata.Jan 21 2016, 6:36 PM

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
461

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

silvas added inline comments.Jan 21 2016, 6:38 PM
include/clang/Frontend/CodeGenOptions.def
107

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

@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).

@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 edited edge metadata.Jan 22 2016, 10:39 AM
davidxl added a subscriber: 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 updated this revision to Diff 45708.Jan 22 2016, 10:48 AM

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

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
3294

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 added inline comments.Jan 22 2016, 12:05 PM
lib/Driver/Tools.cpp
3294

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.

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.

  1. -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 added inline comments.Jan 22 2016, 12:59 PM
lib/Driver/Tools.cpp
3294

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 added inline comments.Jan 22 2016, 1:34 PM
lib/Driver/Tools.cpp
3294

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.

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.

  1. -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 added inline comments.Jan 22 2016, 2:03 PM
lib/CodeGen/BackendUtil.cpp
453

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 added inline comments.Jan 22 2016, 2:41 PM
lib/CodeGen/BackendUtil.cpp
453

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 added inline comments.Jan 25 2016, 12:18 PM
lib/CodeGen/BackendUtil.cpp
453

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 added inline comments.Jan 25 2016, 12:43 PM
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 added a subscriber: xur.Jan 25 2016, 12:52 PM

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 updated this revision to Diff 45929.Jan 25 2016, 4:45 PM
xur marked an inline comment as done.

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 added inline comments.Jan 26 2016, 5:01 PM
lib/Driver/Tools.cpp
5537

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 added a comment.Jan 27 2016, 11:18 AM

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 added inline comments.Jan 27 2016, 11:31 AM
lib/Driver/Tools.cpp
5537

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 updated this revision to Diff 46199.Jan 27 2016, 4:33 PM

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 added inline comments.Jan 27 2016, 4:43 PM
include/clang/Frontend/CodeGenOptions.def
106–107

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 added inline comments.Jan 27 2016, 4:49 PM
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 added inline comments.Jan 27 2016, 5:14 PM
include/clang/Frontend/CodeGenOptions.def
106–107

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–151

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 added a comment.Jan 28 2016, 10:03 AM

I'll send an updated patch shortly.

include/clang/Frontend/CodeGenOptions.def
106–107

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

lib/CodeGen/CodeGenModule.cpp
150–151

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 updated this revision to Diff 46303.Jan 28 2016, 12:04 PM

Integrated most recently comments/suggestions from David and Sean.

Thanks,

-Rong

xur added a comment.Jan 28 2016, 2:34 PM

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 added inline comments.Jan 28 2016, 3:03 PM
lib/Driver/Tools.cpp
3232

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

xur updated this revision to Diff 46389.Jan 29 2016, 9:14 AM

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 added a comment.Jan 29 2016, 11:37 AM

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

xur updated this revision to Diff 46960.Feb 4 2016, 2:09 PM

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 added inline comments.Feb 5 2016, 3:27 PM
include/clang/Frontend/CodeGenOptions.h
252

typo: instrumentation

xur updated this revision to Diff 47251.Feb 8 2016, 2:43 PM
xur marked an inline comment as done.

fixed the typo in comments

xur updated this revision to Diff 47331.Feb 9 2016, 9:21 AM

Ping

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

Thanks,

-Rong

davidxl added inline comments.Feb 18 2016, 10:07 AM
lib/CodeGen/CodeGenModule.cpp
150–151

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

lib/Driver/Tools.cpp
3199

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.

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 marked an inline comment as done.Feb 22 2016, 12:00 PM

I'll send a revised patch soon.

lib/CodeGen/CodeGenModule.cpp
150–151

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
3199

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

test/CodeGen/pgo-instrumentation.c
5

Will do.

xur updated this revision to Diff 48846.Feb 23 2016, 1:04 PM
xur marked an inline comment as done.

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 added inline comments.Feb 23 2016, 3:14 PM
include/clang/Frontend/CodeGenOptions.h
110–118

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 marked 4 inline comments as done.Feb 23 2016, 4:33 PM
xur added inline comments.
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 updated this revision to Diff 48859.Feb 23 2016, 4:51 PM
xur marked 2 inline comments as done.

Integrated with David's comments.

Thanks,

-Rong

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.

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 added a comment.Feb 25 2016, 2:08 PM

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

Thanks,

-Rong

silvas resigned from this revision.Mar 25 2020, 6:36 PM