This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Fine-tuning hash computation for stale matching
ClosedPublic

Authored by spupyrev on Jul 25 2023, 4:46 PM.

Details

Summary

Fine-tuning hash computation for stale matching:

  • introducing a new "loose" basic block hash that allows to match many more blocks than before;
  • tweaking params of the inference algorithm that find (slightly) better solutions;
  • added more meaningful tests for stale matching.

Tested the changes on several open-source benchmarks (clang, rocksdb, chrome)
and one prod workload using different compiler modes (LTO/PGO etc). There is
always an improvement in the quality of inferred profiles.
(The current implementation is still not optimal but the diff is a step forward;
I am open to further suggestions)

Diff Detail

Event Timeline

spupyrev created this revision.Jul 25 2023, 4:46 PM
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
spupyrev retitled this revision from [BOLT] better hashing to [BOLT] Fine-tuning hash computation for stale matching.Jul 25 2023, 4:47 PM
spupyrev edited the summary of this revision. (Show Details)
spupyrev updated this revision to Diff 544554.Jul 26 2023, 4:35 PM

minor tweaks

spupyrev edited the summary of this revision. (Show Details)Jul 27 2023, 12:35 PM
spupyrev updated this revision to Diff 544894.Jul 27 2023, 12:47 PM

fixing tests

spupyrev updated this revision to Diff 544896.Jul 27 2023, 12:52 PM

adding a test check

spupyrev updated this revision to Diff 544949.Jul 27 2023, 3:17 PM

added a test

spupyrev published this revision for review.Jul 27 2023, 3:26 PM

Ready for review now

Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 3:26 PM
Amir added a comment.Aug 1 2023, 1:08 PM

Please fix a couple of nits, LGTM otherwise.

bolt/lib/Core/HashUtilities.cpp
160

As we discussed

bolt/lib/Target/X86/X86MCPlusBuilder.cpp
2875 ↗(On Diff #544949)

This function doesn't do what we wanted it to do (return the short version

bolt/test/X86/reader-stale-yaml.test
41
49
spupyrev updated this revision to Diff 551322.Aug 17 2023, 4:41 PM

Addressing comments by reverting the changes in MCPlusBuilder.h

spupyrev marked 3 inline comments as done.Aug 17 2023, 4:45 PM

Since the last change wasn't trivial, I'll wait for another review before landing this

Amir accepted this revision.Aug 29 2023, 10:11 AM

Looks good. Thank you for updating it.

This revision is now accepted and ready to land.Aug 29 2023, 10:11 AM
This revision was automatically updated to reflect the committed changes.