Page MenuHomePhabricator

[PGO] cc1 option names change for profile instrumentation (NFC)

Authored by xur on Jan 29 2016, 11:35 AM.



This patch is split from

It changes cc1 option -fprofile-generate to an enum option -fprofile-instrument=,
and it takes Clang as the default (and only, or now) value.

It also changes cc1 options -fprofile-generate= to -fprofile-instrument-path=.

The driver level option -fprofile-generate and -fprofile-generate= are intact.

This change will pave the way to integrate new PGO instrumentation in IR level.

Diff Detail

Event Timeline

xur updated this revision to Diff 46400.Jan 29 2016, 11:35 AM
xur retitled this revision from to [PGO] cc1 option names change for profile instrumentation (NFC).
xur updated this object.
xur added reviewers: davidxl, silvas, bogner.
xur added a subscriber: llvm-commits.
davidxl added inline comments.Jan 29 2016, 11:59 AM

Perhaps: "Enable PGO instrumentation. The accepted value is Clang or None"


Remove LLVM

xur updated this revision to Diff 46406.Jan 29 2016, 12:09 PM

Updated the patch with David's comments. Also accept "None" as the -fprofile-instrument= value.


silvas added inline comments.Jan 29 2016, 12:56 PM

This is not accurate. It is not just for PGO.


Keep this as an if.


Is there a way to shorten this comparison? Can we add a helper like hasProfileClangInstr() or something instead of always having to do getProfileInstr() == CodeGenOptions::ProfileClangInstr? Otherwise, many of these comparisons read strange (such as the one in incrementProfileCounter, which is logically checking "is clang instr on or off", but the code as written makes it seem like it asking what kind of instr is being done).


Please use lower case ("clang") for the option value, for consistency with other options.

davidxl edited edge metadata.Jan 29 2016, 1:49 PM

Add a test case for -fprofile-instrument=none and a negative case -fprofile-instrument=Garbage

xur updated this revision to Diff 46438.Jan 29 2016, 3:07 PM
xur edited edge metadata.

Integrated Sean and David review comments:

Note that the new test case to test
-fprofile-instrument=none and -fprofile-instrument=garbage are in tools/clang/test/Profile/c-generate.c.



This looks quite clean, but please wait for approval from one of other reviewers.

silvas added inline comments.Jan 29 2016, 6:01 PM

Why are you not using %clang_cc1 consistently here?


|& doesn't seem to be used in other tests inside clang and llvm. A more portable alternative is probably required to pass the bots.

davidxl added inline comments.Jan 29 2016, 7:04 PM

Should be safe to use 2>&1 | FileCheck -- there are existing test cases ..

xur updated this revision to Diff 46477.Jan 30 2016, 3:09 PM

change test case test/Profile/c-generate.c:
(1) use clang_cc1
(2) use 2>&1



silvas accepted this revision.Feb 5 2016, 6:38 PM
silvas edited edge metadata.

Tiny nit, but this LGTM as well.


This : is inconsistently placed. (also for some of the other tablegen changes)

This revision is now accepted and ready to land.Feb 5 2016, 6:38 PM
xur added a comment.Feb 8 2016, 10:17 AM

Thanks Sean. I committed r260116 to fix the bad format for ':'.


silvas closed this revision.Jul 8 2016, 11:47 PM