Page MenuHomePhabricator

[CSSPGO] Fix dangling context strings and improve profile order consistency and error handling
ClosedPublic

Authored by wenlei on Wed, Apr 7, 11:13 PM.

Details

Summary

This patch fixed the follow issues along side with some refactoring:

  1. Fix bugs where StringRef for context string out live the underlying std::string. We now keep string table in profile generator to hold std::strings. We also do the same for bracketed context strings in profile writer.
  2. Make sure profile output strictly follow (total sample, name) order. Previously, there's inconsistency between ProfileMap's key and FunctionSamples's name, leading to inconsistent ordering. This is now fixed by introducing context profile canonicalization. Assertions are also added to make sure ProfileMap's key and FunctionSamples's name are always consistent.
  3. Enhanced error handling for profile writing to make sure we bubble up errors properly for both llvm-profgen and llvm-profdata when string table is not populated correctly for extended binary profile.
  4. Keep all internal context representation bracket free. This avoids creating new strings for context trimming, merging and preinline. getNameWithContext API is now simplied accordingly.
  5. Factor out the code for context trimming and merging into SampleContextTrimmer in SampleProf.cpp. This enables llvm-profdata to use the trimmer when merging profiles. Changes in llvm-profgen will be in separate patch.

Diff Detail

Event Timeline

wenlei created this revision.Wed, Apr 7, 11:13 PM
wenlei requested review of this revision.Wed, Apr 7, 11:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Apr 7, 11:13 PM
hoy added a comment.Thu, Apr 8, 10:19 AM

Thanks for fixing the name volatileness issue which was overlooked previously.

llvm/lib/ProfileData/SampleProf.cpp
385

For a profile whose key is different from the profile name string, are there other profiles merged into it, or is it merged into other profiles? A comment could be helpful.

llvm/lib/ProfileData/SampleProfWriter.cpp
503

Can brackets be output here instead of adding it to name table and looking it up there?

wenlei added inline comments.Thu, Apr 8, 10:35 AM
llvm/lib/ProfileData/SampleProf.cpp
385

It could be a profile from context merging, see line 358. For pre-inliner, it could also be promoted context profile without merging. Will add comment.

llvm/lib/ProfileData/SampleProfWriter.cpp
503

Not every name needs bracket. One example is the call target name for call sites. If we do it here, we can't differentiate between such cases and the full context names used in profile header.

wenlei updated this revision to Diff 336165.Thu, Apr 8, 10:40 AM

Update comments.

hoy added inline comments.Thu, Apr 8, 11:05 AM
llvm/lib/ProfileData/SampleProf.cpp
338

Nit: cold profile

388

Still trying to understand. So for cold profile trim, what kind of a profile can be here. In trimAndMergeColdContextProfiles all cold profiles are removed and all based profiles are added into ProfileMap.

llvm/lib/ProfileData/SampleProfWriter.cpp
503

I see. Yeah, unfortunately we cannot tell which one needs brackets or not.

wenlei marked an inline comment as done.Thu, Apr 8, 11:59 AM
wenlei added inline comments.
llvm/lib/ProfileData/SampleProf.cpp
388

Cold profile trimming is simply removing cold profiles, so it won't lead to inconsistency between profile map key and actual profile's context. Actually now looking at the merging part again, the currently implementation doesn't require canonicalization either as the new base profile has the correct key.

I'm removing canonicalization from trimAndMergeColdContextProfiles, but pre-inliner still needs it.

wenlei updated this revision to Diff 336187.Thu, Apr 8, 12:00 PM

Address Hongtao's comments.

hoy accepted this revision.Thu, Apr 8, 12:25 PM

LGTM, thanks.

This revision is now accepted and ready to land.Thu, Apr 8, 12:25 PM