Page MenuHomePhabricator

[CSSPGO][llvm-profgen] Change sample count of dangling probe in llvm-profgen
ClosedPublic

Authored by wlei on Feb 16 2021, 1:03 PM.

Details

Summary

Change to use UINT64_MAX for sample count of dangling probe from llvm-profgen side. The compiler will identify this for further process, please refer to https://reviews.llvm.org/D95962 for the details.

Diff Detail

Event Timeline

wlei created this revision.Feb 16 2021, 1:03 PM
wlei requested review of this revision.Feb 16 2021, 1:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2021, 1:03 PM
wlei edited the summary of this revision. (Show Details)Feb 16 2021, 1:07 PM
wlei added reviewers: hoy, wmi, wenlei, davidxl.
wenlei added inline comments.Feb 16 2021, 9:27 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
506

Do we still want to count samples on dangling probe towards total samples? Without deduplication for dangling probes, we could have multiple dangling probes in the same block and counting samples covering these probe repeatedly may cause bloated total samples?

hoy added inline comments.Feb 16 2021, 10:40 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
506

Good point. Since the samples collected on dangling probes are invalid, I would not count it against total samples.

wlei added inline comments.Feb 16 2021, 10:49 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
506

I see, thanks for your suggestion.

How about the count for isEntry for line506, I guess it's the original count not the zero? Or we won't have dangling probe which is the entry probe?

hoy added inline comments.Feb 16 2021, 11:06 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
506

That's a good point. It's possible to have a dangling entry probe and we should bail out here. Or we can just return right after adding body samples for dangling probes.

wlei updated this revision to Diff 324339.Feb 17 2021, 9:58 AM

remove count towards total sample for dangling probes

llvm/tools/llvm-profgen/ProfileGenerator.cpp
506

I see. Since the sample count are invalid, we shouldn't count for total sample nor using it to infer inliner's sample count. Thanks for your clarification.

hoy added inline comments.Feb 26 2021, 10:13 AM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
501

Actually I UINT64_MAX may cause overflow to total samples. Even if it doesn't, profile merge may overflow too. That's one of the reasons we were using 0 as a special count. @wmi What do you think about keeping using 0?

hoy added inline comments.Mar 3 2021, 12:17 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
501

I talked to other folks and they like using UINT64_MAX instead of 0 to be less confusing. @wlei we may need to fix the places that accumulate total samples and set entry count to not using UINT64_MAX as the sample count.

wlei added inline comments.Mar 3 2021, 5:46 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
501

Thanks for the sharing. So for profgen side, we can keep this patch, right? see line 501, it only adds the body sample and doesn't accumulate the total sample.

For compiler side, we need to avoid the addition when meeting the UINT64_MAX

hoy added inline comments.Mar 3 2021, 6:24 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
501

May need to check dangling in CSProfileGenerator::populateFunctionBoundarySamples when updating the callsite target samples?

hoy accepted this revision.Mar 3 2021, 6:30 PM
hoy added inline comments.
llvm/tools/llvm-profgen/ProfileGenerator.cpp
501

I was wrong. The current implementation looks good. We are not propagating UINT64_MAX anywhere except for adding it as a body sample.

On the compiler side, the merging of dangling probes are taken care of here: https://reviews.llvm.org/D95962 .

Please rebase this change on top of D95962 to share the definition of FunctionSamples::InvalidProbeCount.

This revision is now accepted and ready to land.Mar 3 2021, 6:30 PM
wlei added inline comments.Mar 3 2021, 8:56 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
501

Thanks for your clarification. Will rebase it after D95962 landed.

wenlei added inline comments.Mar 3 2021, 11:55 PM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
501

Since we are using addBodySamples here, can we have probe count not equal to UINT64_MAX after the addition? Can we add assertion?

wlei updated this revision to Diff 328043.Mar 4 2021, 12:12 AM

address reviewers' feedback

wlei added inline comments.Mar 4 2021, 12:13 AM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
501

Good catch. Changed to update the count only it doesn't exist and add the assertion.

wenlei accepted this revision.Mar 4 2021, 12:15 AM

lgtm.

hoy added inline comments.Mar 4 2021, 12:31 AM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
501

This is a good point. Probes with same ID may have both dangling and non-dangling copies. Using SampleRecord::merge should be safe, like R->merge(unctionSamples::InvalidProbeCount, 1)

wlei updated this revision to Diff 328224.Mar 4 2021, 10:07 AM

use merge function to update body samples for probe

wlei added inline comments.Mar 4 2021, 10:13 AM
llvm/tools/llvm-profgen/ProfileGenerator.cpp
501

Thanks for your reminding. I see, then we should also consider non-dangling case, so added a helper function addBodySamplesForProbe for this. See whether this looks good for you?

hoy accepted this revision.Mar 4 2021, 10:13 AM

lgtm, thx!

This revision was landed with ongoing or failed builds.Mar 8 2021, 2:37 PM
This revision was automatically updated to reflect the committed changes.