This is an archive of the discontinued LLVM Phabricator instance.

[PGO] Fix two issues in PGOMemOPSizeOpt.
ClosedPublic

Authored by hjyamauchi on Feb 26 2021, 3:23 PM.

Details

Summary
  1. PGOMemOPSizeOpt grabs only the first, up to five (by default) entries from

the value profile metadata and preserves the remaining entries for the fallback
memop call site. If there are more than five entries, the rest of the entries
would get dropped. This is fine for PGOMemOPSizeOpt itself as it only promotes
up to 3 (by default) values, but potentially not for other downstream passes
that may use the value profile metadata.

  1. PGOMemOPSizeOpt originally assumed that only values 0 through 8 are kept

track of. When the range buckets were introduced, it was changed to skip the
range buckets, but since it does not grab all entries (only five), if some range
buckets exist in the first five entries, it could potentially cause fewer
promotion opportunities (eg. if 4 out of 5 were range buckets, it may be able to
promote up to one non-range bucket, as opposed to 3.) Also, combined with 1, it
means that wrong entries may be preserved, as it didn't correctly keep track of
which were entries were skipped.

To fix this, PGOMemOPSizeOpt now grabs all the entries (up to the maximum number
of value profile buckets), keeps track of which entries were skipped, and
preserves all the remaining entries.

Diff Detail

Event Timeline

hjyamauchi created this revision.Feb 26 2021, 3:23 PM
hjyamauchi requested review of this revision.Feb 26 2021, 3:23 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 26 2021, 3:23 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Perhaps add a test case showing that up to 3 ranges can be promoted?

Perhaps add a test case showing that up to 3 ranges can be promoted?

Do you mean one in general or for 2 (specially this part "if 4 out of 5 were range buckets, it may be able to
promote up to one non-range bucket, as opposed to 3.")?

hjyamauchi updated this revision to Diff 327312.Mar 1 2021, 4:13 PM

Address comment.

Okay added a new test.

davidxl added inline comments.Mar 3 2021, 4:03 PM
compiler-rt/include/profile/InstrProfData.inc
863

should we define a macro for it?

hjyamauchi marked an inline comment as done.

Address comment.

This revision is now accepted and ready to land.Mar 10 2021, 11:34 AM
This revision was landed with ongoing or failed builds.Mar 11 2021, 9:53 AM
This revision was automatically updated to reflect the committed changes.