This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

This patch is split from http://reviews.llvm.org/D15829.

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
include/clang/Driver/CC1Options.td
272

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

lib/Frontend/CompilerInvocation.cpp
481

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.

-Rong

silvas added inline comments.Jan 29 2016, 12:56 PM
include/clang/Frontend/CodeGenOptions.def
106–107

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

lib/CodeGen/BackendUtil.cpp
432–433

Keep this as an if.

lib/CodeGen/CGStmt.cpp
1207–1208

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

lib/Frontend/CompilerInvocation.cpp
485

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.

Thanks,

-Rong

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

silvas added inline comments.Jan 29 2016, 6:01 PM
test/Profile/c-generate.c
8

Why are you not using %clang_cc1 consistently here?

9

|& 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
test/Profile/c-generate.c
9

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

Thanks,

-Rong

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

Tiny nit, but this LGTM as well.

include/clang/Basic/DiagnosticDriverKinds.td
30

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 ':'.

-Rong

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