This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Add stale-related logging
ClosedPublic

Authored by spupyrev on Jul 7 2023, 11:56 AM.

Details

Summary

Adding some logs related to stale profile matching. The new data can be helpful
to understand how "stale" the input profile is and how well the inference is
able to utilize the stale data.

Example of outputs on clang-10 built with LTO (profile collected on a year-old release):

BOLT-INFO: inferred profile for 2101 (18.52% of profiled, 100.00% of stale) functions responsible for 30.95% samples (14754697 out of 47670654)
BOLT-INFO: stale inference matched 89.42% of basic blocks (79052 out of 88402 stale) responsible for 76.99% samples (645737 out of 838719 stale)

LTO+AutoFDO:

BOLT-INFO: inferred profile for 6146 (57.57% of profiled, 100.00% of stale) functions responsible for 90.34% samples (50891403 out of 56330313)
BOLT-INFO: stale inference matched 74.55% of basic blocks (191295 out of 256589 stale) responsible for 57.30% samples (1288632 out of 2248799 stale)

Diff Detail

Event Timeline

spupyrev created this revision.Jul 7 2023, 11:56 AM
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 edited the summary of this revision. (Show Details)Jul 7 2023, 12:01 PM
spupyrev published this revision for review.Jul 7 2023, 12:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2023, 12:02 PM
Amir added a comment.Jul 13 2023, 10:33 AM

LGTM.
@maksfb – please take a look if it's fine to add BF Inference stats and methods.

Amir accepted this revision.Jul 13 2023, 10:33 AM
This revision is now accepted and ready to land.Jul 13 2023, 10:33 AM
Amir retitled this revision from [BOLT] Adding stale-related logging to [BOLT] Add stale-related logging.Jul 13 2023, 10:33 AM
maksfb requested changes to this revision.Jul 13 2023, 11:42 AM
maksfb added inline comments.
bolt/include/bolt/Core/BinaryFunction.h
394–403

Let's move the stats out of the BinaryFunction. Once the input function list is populated, function addresses are fixed. You can use a map, e.g., to attach stats to BinaryFunction *. I'm also not sure you need per-function stats or having aggregated is enough.

This revision now requires changes to proceed.Jul 13 2023, 11:42 AM
spupyrev added inline comments.Jul 13 2023, 12:19 PM
bolt/include/bolt/Core/BinaryFunction.h
394–403

Yes that's what I was thinking on doing. Is there a good example of storing stats or some other info across different passes?

maksfb added inline comments.Jul 13 2023, 1:41 PM
bolt/include/bolt/Core/BinaryFunction.h
394–403

We don't have a good stats aggregator in BOLT. The easiest way is to create a new class and include its instance in the BC. Another way is to modify the pass manager, which is more involved. Also worth checking what LLVM has implemented.

spupyrev added inline comments.Jul 14 2023, 9:33 AM
bolt/include/bolt/Core/BinaryFunction.h
394–403

BC seems to already have something similar, e.g., BinaryContext::MissedMacroFusionPairs, although that's not a separate object.
LLVM has ADT/Statistic.h which looks similar but implemented via static variables so some changes would be needed to access the stats in different passes.

I guess the best would be to have a stat wrapper in BC and move existing fields (MissedMacroFusionPairs, MissedMacroFusionExecCount) there?

spupyrev updated this revision to Diff 540540.Jul 14 2023, 12:53 PM
spupyrev edited the summary of this revision. (Show Details)

Moving stats from BinaryFunction to (aggregated) BinaryContext

maksfb added inline comments.Jul 14 2023, 5:03 PM
bolt/include/bolt/Core/BinaryFunction.h
394–403

Sounds reasonable to me.

maksfb added inline comments.Jul 14 2023, 5:05 PM
bolt/lib/Profile/StaleProfileMatching.cpp
401–402

Now you can pass just the BinaryContext.

spupyrev updated this revision to Diff 541783.Jul 18 2023, 4:46 PM
  • wrapping binary stats into a struct;
  • printing matching stats based on strong hashes, where we have high confidence in correctness.
spupyrev updated this revision to Diff 541785.Jul 18 2023, 4:52 PM

another typo

Amir added inline comments.Jul 19 2023, 12:05 PM
bolt/lib/Profile/StaleProfileMatching.cpp
449

But AllInstrHashes is not used in FlowBlock lookup above?

spupyrev updated this revision to Diff 544098.Jul 25 2023, 2:29 PM

Updated the part computing matching metrics to make it more readable. Let me know if there is still confusion.

maksfb accepted this revision.Jul 26 2023, 8:14 AM

LGTM with a couple of nits. Thanks!

bolt/include/bolt/Core/BinaryContext.h
650
654
This revision is now accepted and ready to land.Jul 26 2023, 8:14 AM
This revision was landed with ongoing or failed builds.Jul 27 2023, 8:57 AM
This revision was automatically updated to reflect the committed changes.