Page MenuHomePhabricator

[PGO] Instrument function entry BB by default in IR PGO
Needs ReviewPublic

Authored by xur on Jul 1 2020, 11:34 PM.

Details

Reviewers
davidxl
wmi
Summary

This patch enables the instrumentation function entry BB by default in
IR PGO. This will slow down the instrumentation for some cases.
One can use the internal option "-mllvm -pgo-instrument-entry=false"
to disable it.

For single threaded programs, the slowdown should be very minimum.

The benefit of always instrumenting function entry BB is that we
can know the function entry count by form the profile and this often
facilitates some profile offline processing.

This patch also bumps up the index profile version, from 6 to 7.
Strictly speaking, this is not needed -- the instrumentation order depends
on the implementation and there is no guarantee of the instrumentation order.
We bump this up just to prevent some potential breakages (performance wise)
for people using some stored (old) profiles.

This patch will not change the behavior if the profile version is lower
than the new version (7) -- unless the internal option is explicitly specified.

For the index profile with version 7 or above, This patch assumes that the
function entry BB by default. Again the internal option
"-mllvm -pgo-instrument-entry" takes precedence over the index profile version.

Diff Detail

Event Timeline

xur created this revision.Jul 1 2020, 11:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2020, 11:34 PM

The version number probably can be kept unchanged -- however there needs to be a new variant bit to indicate the choice.

Changing version number does not work either -- suppose after the flip, user select the instr-entry=false, but the version number will still be 7 -- this will lead to problem at profile-use time.

Here version is overloaded with different meanings: 1) to indicate format change; 2) to indicate instrumentation compiler version that is capable of producing this format. These two purposes can be contradicting to each other as the user can use option to force the old format with the new compiler.

I am not sure how compiler can relax the check at the profile use time.

wmi added a comment.Jul 6 2020, 9:21 AM

Here version is overloaded with different meanings: 1) to indicate format change; 2) to indicate instrumentation compiler version that is capable of producing this format. These two purposes can be contradicting to each other as the user can use option to force the old format with the new compiler.

For the case new compiler + -pgo-instrument-entry=false, if -pgo-instrument-entry=false is used in both profile-gen and profile-use explicitly, we won't have a problem. Do we have a senario which it doesn't work?

I am not sure how compiler can relax the check at the profile use time.

I just checked IndexedInstrProfReader::readHeader and the version check part has already allowed the case with profile version being smaller than compiler current format version so the relax I am talking about is already there -- Rong's patch has already covered my proposed solution.

If we require profile-use also to use the option, it will work, but I think it is better and more convenient to change variant bit (I believe there are plenty). Bumping version can potentially complicate things in the future.

We should probably also add a new directive in text format for the variant, something like

:entry_first

When the directive is lacking, it means the old variant.

xur updated this revision to Diff 278829.Jul 17 2020, 10:05 AM

Changed the implementation based on review comments.
Now we will not change the version of the index profile.
We will use bit 58 in the profile header to indicate if
we always instrument the entry block. This applies
to both raw and index format.

For the text formation, we add two new directives
:entry_first
:not_entry_first
When omitted, it's the same as :not_entry_first.

Also changed llvm-profdata to dump related information.

Rong, please split the patch that handles variant and text dump support without changing the default first.

xur updated this revision to Diff 280984.Jul 27 2020, 11:16 AM

Update the patch after committing the support in a separated patch:
commit 50da55a58534e9207d8d5e31c8b4b5cf0c624175

This patch will turn -pgo-instrument-entry to true by default.

Many tests are already submitted with the supporting patch.
This patch will update the rest of tests.

I'm not confident that the test changes are complete as I only tested X86 Linux platforms.