This is an archive of the discontinued LLVM Phabricator instance.

[CSSPGO] Support of CS profiles in extended binary format.
ClosedPublic

Authored by hoy on Jan 27 2021, 10:45 AM.

Details

Summary

This change brings up support of context-sensitive profiles in the format of extended binary. Existing sample profile reader/writer/merger code is being tweaked to reflect the fact of bracketed input contexts, like ([...]). The paired brackets are also needed in extbinary profiles because we don't yet have an otherwise good way to tell calling contexts apart from regular function names since the context delimiter @ can somehow serve as a part of the C++ mangled names.

Diff Detail

Event Timeline

hoy created this revision.Jan 27 2021, 10:45 AM
hoy requested review of this revision.Jan 27 2021, 10:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2021, 10:45 AM
hoy retitled this revision from [CSSPGO] Support of extended-binary-formatted CS profiles. to [CSSPGO] Support of CS profiles in extended binary format. .Jan 27 2021, 10:48 AM
hoy edited the summary of this revision. (Show Details)
hoy added reviewers: wmi, davidxl, wenlei.
wenlei added inline comments.Jan 27 2021, 12:05 PM
llvm/include/llvm/ProfileData/SampleProf.h
445

What about using getNameWithContext(bool WithBracket = false), also change InputContext to FullContextWithBracket.

760

same here, merge with getNameWithContext(bool WithBracket=false)?

llvm/lib/ProfileData/SampleProfReader.cpp
323

Use only one counter, and assert(CSProfileCount == 0 || CSProfileCount == Profiles.size()) ?

llvm/lib/ProfileData/SampleProfWriter.cpp
150

If we keep StringRef for context with bracket, we can use it for text profile writer as well.

As we discussed off patch, the context provided by llvm-profgen may not have [], and it's probably better to fix that to make it consistent.

llvm/test/tools/llvm-profdata/Inputs/csspgo-profile.proftext
1 ↗(On Diff #319623)

Add a function without calling context? We can reuse the test case from our fork?

llvm/test/tools/llvm-profdata/merge-cs-profile.test
1 ↗(On Diff #319623)

What about adding a roundtrip conversion test for text and extbin format?

hoy marked an inline comment as done.Jan 27 2021, 12:37 PM
hoy added a subscriber: wlei.
hoy added inline comments.
llvm/include/llvm/ProfileData/SampleProf.h
445

That looks better. Thanks.

llvm/lib/ProfileData/SampleProfReader.cpp
323

Yeah, that's smart.

llvm/lib/ProfileData/SampleProfWriter.cpp
150

Exactly. The text writer for now has to emit the brackets explicitly like below. @wlei this can be fixed by including brackets in the final context string during profile generation.

std::error_code SampleProfileWriterText::writeSample(const FunctionSamples &S) {
  auto &OS = *OutputStream;
  if (FunctionSamples::ProfileIsCS)
    OS << "[" << S.getNameWithContext() << "]:" << S.getTotalSamples();
  else
    OS << S.getName() << ":" << S.getTotalSamples();
llvm/test/tools/llvm-profdata/Inputs/csspgo-profile.proftext
1 ↗(On Diff #319623)

Sounds good, will port our internal test.

llvm/test/tools/llvm-profdata/merge-cs-profile.test
1 ↗(On Diff #319623)

It's actually done by line 4.

hoy updated this revision to Diff 319647.Jan 27 2021, 12:45 PM
hoy retitled this revision from [CSSPGO] Support of CS profiles in extended binary format. to [CSSPGO] Support of CS profiles in extended binary format..

Addressing Wenlei's feedbacks.

wenlei accepted this revision.Jan 27 2021, 12:50 PM

lgtm, with one nit.

llvm/include/llvm/ProfileData/SampleProf.h
445

missed the rename of InputContext to FullContextWithBracket?

This revision is now accepted and ready to land.Jan 27 2021, 12:50 PM
hoy updated this revision to Diff 319667.Jan 27 2021, 1:59 PM

Addressing nit.

wlei added inline comments.Jan 27 2021, 2:05 PM
llvm/lib/ProfileData/SampleProfWriter.cpp
150

Got it! So I will fix like the code below:

if (FunctionSamples::ProfileIsCS)
  OS << S.getNameWithContext()  << S.getTotalSamples();
else
  OS << S.getName() << ":" << S.getTotalSamples();
wlei added inline comments.Jan 27 2021, 2:34 PM
llvm/include/llvm/ProfileData/SampleProf.h
445

I didn't see any where in this patch add the bracket. Do we need to add it in this patch? because the input ContextStr actually doesn't have the bracket, the bracket in the regression test is printed by the text format which added the bracket in the end, but not for extended binary.

hoy added inline comments.Jan 27 2021, 2:43 PM
llvm/include/llvm/ProfileData/SampleProf.h
445

This patch doesn't add brackets. It relies on bracketed input, like a real text profile, or in-memory profiles fed by llvm-profgen. Currently when llvm-profgen creates a samplecontext , it doesn't add brackets. Instead, it sets FunctionSamples::ProfileIsCS explicitly. However, the context should have brackets as well so that the the contextness can be inferred automatically without setting ProfileIsCS.

llvm/lib/ProfileData/SampleProfWriter.cpp
150

Thanks. Ideally it should be just OS << S.getNameWithContext(true) << S.getTotalSamples(); unconditionally once brackets are added to contexts on the llvm-profgen side.

wlei added inline comments.Jan 27 2021, 3:17 PM
llvm/include/llvm/ProfileData/SampleProf.h
445

Thanks for your explanation, that makes a lot of sense.

So we need to change the llvm-profgen side, to add the brackets for the input ContextStr, then we don't need to relay on the text format, also that's the right way for extended format.

454

Should we merge it to the condition in line 462. because as its name "WithBracket", it should always have bracket or we can add a check for the HasContext.

if (HasContext) {
  FullContextWithBracket = ContextStr;
}
hoy added inline comments.Jan 27 2021, 3:48 PM
llvm/include/llvm/ProfileData/SampleProf.h
454

Good point. It really refers to the original input context so that we can use it unconditionally for non-CS profile as well. @wenlei thinking about renaming it OrigContext, what do you think?

wenlei added inline comments.Jan 27 2021, 3:52 PM
llvm/include/llvm/ProfileData/SampleProf.h
454

Good catch, in this case OrigContext or InputContext makes more sense.. Sorry for the confusion.

wmi added inline comments.Jan 27 2021, 4:08 PM
llvm/include/llvm/ProfileData/SampleProf.h
442

Nit: I feel getNameWithoutContext is more straightforward.

685–687

Is it possible for the two FunctionSamples candidates to have different non empty contexts?

llvm/lib/ProfileData/SampleProfReader.cpp
547

This is unnecessary. The next statement will create the default entry if the key doesn't exist in the map.

hoy added inline comments.Jan 27 2021, 4:16 PM
llvm/include/llvm/ProfileData/SampleProf.h
442

you mean for the getName function?

454

Sound good. Renamed to InputContext.

685–687

This merge function is called under the assumption that Other has the same context with this or this is empty, which basically means we are initializing this with Other.

llvm/lib/ProfileData/SampleProfReader.cpp
547

Good catch. Will remove.

wmi added inline comments.Jan 27 2021, 4:32 PM
llvm/include/llvm/ProfileData/SampleProf.h
442

Yes.

hoy added inline comments.Jan 27 2021, 4:50 PM
llvm/include/llvm/ProfileData/SampleProf.h
442

Sounds good.

hoy updated this revision to Diff 319718.Jan 27 2021, 4:51 PM

Addressing Wei's feedbacks.

wmi accepted this revision.Jan 27 2021, 7:05 PM

LGTM.

This revision was landed with ongoing or failed builds.Jan 27 2021, 9:30 PM
This revision was automatically updated to reflect the committed changes.