This is an archive of the discontinued LLVM Phabricator instance.

[PGO] Supporting code for always instrumenting entry block
ClosedPublic

Authored by xur on Jul 21 2020, 12:07 PM.

Details

Summary

This patch is a split of D83024 (https://reviews.llvm.org/D83024)

It includes all the supporting code without flipping the default behavior.
It adds a variant bit in the profile version, adds new directives in text profile format, and changes llvm-profdata tool accrodingly.

Many test changes from D83024 are also included. This should be OK as I explicitly specify the options.

Diff Detail

Event Timeline

xur created this revision.Jul 21 2020, 12:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2020, 12:07 PM
davidxl added inline comments.Jul 22 2020, 9:30 AM
clang/test/CodeGenCXX/Inputs/profile-remap.proftext
3 ↗(On Diff #279601)

is this change needed?

llvm/lib/ProfileData/InstrProfReader.cpp
188

The parser should probably be made such that the order of the directive does not matter.

xur marked 2 inline comments as done.Jul 22 2020, 10:24 AM
xur added inline comments.
clang/test/CodeGenCXX/Inputs/profile-remap.proftext
3 ↗(On Diff #279601)

They are not needed.

This actually tests the new proftext file.

We can remove it and the change in gcc-flag-compatibility_IR.proftext
Or add update the test to use the new proftext.

llvm/lib/ProfileData/InstrProfReader.cpp
188

That can be done.

How about duplicates? if the order does not matter, I assume we can have multiple directive and the last one rules?
Or we need to throw an error when seeing duplicates?

xur updated this revision to Diff 279904.Jul 22 2020, 12:01 PM

Addressed review comments from David.

davidxl accepted this revision.Jul 22 2020, 12:48 PM

lgtm (with adding a not_entry_first test case).

This revision is now accepted and ready to land.Jul 22 2020, 12:48 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2020, 3:03 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kpdev42 added inline comments.
llvm/include/llvm/ProfileData/InstrProfData.inc
676

This revision is closed, so excuse me for the question:
Files ./llvm/include/llvm/ProfileData/InstrProfData.inc and ./compiler-rt/include/profile/InstrProfData.inc should be two identical copies, as stated in their description.

* The file has two identical copies. The master copy lives in LLVM and
* the other one  sits in compiler-rt/lib/profile directory. To make changes
* in this file, first modify the master copy and copy it over to compiler-rt.
* Testing of any change in this file can start only after the two copies are
* synced up.

Should we add VARIANT_MASK_INSTR_ENTRY to compiler-rt or change description?