Page MenuHomePhabricator

[SampleFDO] Make sample profile loader unaware of compact format change
ClosedPublic

Authored by wmi on Sep 4 2018, 9:52 AM.

Details

Summary

The patch tries to make sample profile loader independent of profile format change. It moves compact format related code into FunctionSamples and SampleProfileReader classes, and sample profile loader only have to interact with those two classes and will be unaware of profile format changes.

The cleanup also contain some fixes to further remove the difference between compactbinary format and binary format. After the cleanup using different formats originated from the same profile will generate exactly the same binaries, which we verified by compiling two large server benchmarks w/wo thinlto.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi created this revision.Sep 4 2018, 9:52 AM
davidxl added inline comments.Sep 5 2018, 9:06 AM
include/llvm/ProfileData/SampleProf.h
461 ↗(On Diff #163853)

Add comments on this code about the substr mapping.

test/Transforms/SampleProfile/Inputs/function_metadata.prof
1 ↗(On Diff #163853)

why changing the profile data?

wmi added inline comments.Sep 5 2018, 9:22 AM
include/llvm/ProfileData/SampleProf.h
461 ↗(On Diff #163853)

Ok, will do.

test/Transforms/SampleProfile/Inputs/function_metadata.prof
1 ↗(On Diff #163853)

Because in function_metadata.prof, the total sample count of a function doesn't equal to the added up count of all the samples in the function body. SampleProfileReader will not care about the inconsistency and will use the incorrect total sample count directly.

However, the autofdo dumping and other conversion tool will discard the incorrect total count and recompute it according to the added up count of samples inside function body.

The test was to verify different formats converted from the same profile will generate the same results, so I needed to make the total sample count consistent with how it is computed.

wmi updated this revision to Diff 164073.Sep 5 2018, 10:49 AM

Add comment for substr mapping.

davidxl added inline comments.Sep 6 2018, 10:05 AM
include/llvm/ProfileData/SampleProf.h
464 ↗(On Diff #164073)

why are those suffixes stripped in sample profile?

wmi added inline comments.Sep 6 2018, 10:23 AM
include/llvm/ProfileData/SampleProf.h
464 ↗(On Diff #164073)

It is possible that the binary sampled is built in thinlto mode and some functions are promoted. The profile generated from such binary will contain names after promotion. If such profile is used in thinlto pre-link-phase or nonthinlto build, we need to strip those names from sample before we can find a match in the module.

Indeed if there are multiple local functions with the same name are promoted, their samples will be merged together after the strip, which will introduce some imprecision. Maybe this part can be improved -- don't strip names from sample, but always promote the local names in the module before trying to match it with the name from sample. I am not sure whether the effort is worthy. Dehao may already evaluate it before?

danielcdh added inline comments.Sep 6 2018, 10:30 AM
include/llvm/ProfileData/SampleProf.h
464 ↗(On Diff #164073)

I remember that name stripping is important as there are many different types of suffixes added to the function name which makes it impossible to match. But I do not remember the exact impact. It may be worth re-evaluate.

For the local functions, what's the promoted name look like after thinlto? If it's deterministic, then we should not strip it.

davidxl added inline comments.Sep 6 2018, 10:31 AM
include/llvm/ProfileData/SampleProf.h
464 ↗(On Diff #164073)

How are local functions handled without thinlto for samplePGO? Instrumentatation PGO always qualify the name with module name.

danielcdh added inline comments.Sep 6 2018, 10:34 AM
include/llvm/ProfileData/SampleProf.h
464 ↗(On Diff #164073)

They'll just share the function name have all the profiles merged, which I agree is bad, but I don't think we have a way to identify which module this function is coming from.

davidxl added inline comments.Sep 6 2018, 10:53 AM
include/llvm/ProfileData/SampleProf.h
464 ↗(On Diff #164073)

Since this issue is not something new, we can defer this later.

davidxl accepted this revision.Sep 6 2018, 2:29 PM

lgtm

This revision is now accepted and ready to land.Sep 6 2018, 2:29 PM
danielcdh accepted this revision.Sep 6 2018, 2:36 PM
This revision was automatically updated to reflect the committed changes.