Page MenuHomePhabricator

[CSSPGO] Update pseudo probe distribution factor based on inline context.
ClosedPublic

Authored by hoy on May 13 2021, 11:07 AM.

Details

Summary

With prelink inlining, pseudo probes with same ID can come from different inline contexts. Such probes should not share samples and their factors should be fixed up separately.

I'm seeing 0.3% speedup for SPEC2017 overall. Benchmark 631.deepsjeng_s benefits the most, about 4%.

Diff Detail

Event Timeline

hoy created this revision.May 13 2021, 11:07 AM
hoy requested review of this revision.May 13 2021, 11:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2021, 11:07 AM
wmi added inline comments.May 14 2021, 9:30 AM
llvm/include/llvm/IR/PseudoProbe.h
87

InlineAt saves the pointer of DILocation which may become dangling easily, so the current intention is to use it in a single pass PseudoProbeUpdatePass. If it is only used in a single pass, can we use a separate map to save the information instead of adding a field in PseudoProbe, so the fields in struct PseudoProbe all contain persistent information.

hoy added inline comments.May 14 2021, 9:58 AM
llvm/include/llvm/IR/PseudoProbe.h
87

Good point! Yes, inside PseudoProbeUpdatePass we can use an auxiliary map to save InlineAt. However, there's the PseudoProbeVerifier pass that tracker probe duplication throughout the pipeline and we need a globally stable InlineAt there. Since that pass is for debugging only, maybe we can just use a computed a hash for inline stack string though it's a bit slow?

wenlei added inline comments.May 14 2021, 10:35 AM
llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
141–144

is getValue a new member of PseudoProbe struct? didn't find the definition.

hoy added inline comments.May 14 2021, 10:37 AM
llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
141–144

getValue is a method of Optional. It returns the underlying PseudoProbe object.

wenlei added inline comments.May 14 2021, 10:41 AM
llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
141–144

ah, nvm. didn't look careful enough. :)

wlei added a comment.May 14 2021, 11:25 AM

Try to understand this:

So we merge the samples from the same function but different context, does this cause the issue?

Such probes should not share samples and their factors should be fixed up separately.

So with that, when code duplication happens, we need to fix probe factor, so here it will do it separately by grouping the probe by different inline context. then each group work for one context inlining. is my understanding correct?

hoy added a comment.May 14 2021, 11:38 AM

Try to understand this:

So we merge the samples from the same function but different context, does this cause the issue?

Exactly.

Such probes should not share samples and their factors should be fixed up separately.

So with that, when code duplication happens, we need to fix probe factor, so here it will do it separately by grouping the probe by different inline context. then each group work for one context inlining. is my understanding correct?

That's right. Probe in a function profile can have multiple copies due to code duplication. We just want to share the corresponding samples among these duplications, but not among all probes with same id but from different function profiles.

wmi added inline comments.May 14 2021, 2:33 PM
llvm/include/llvm/IR/PseudoProbe.h
87

Sounds good. Likely computing a hash for inline stack won't be very slow, then it can be used in both PseudoProbeUpdatePass and PseudoProbeVerifier.

wlei added a comment.May 14 2021, 2:42 PM

Try to understand this:

So we merge the samples from the same function but different context, does this cause the issue?

Exactly.

Such probes should not share samples and their factors should be fixed up separately.

So with that, when code duplication happens, we need to fix probe factor, so here it will do it separately by grouping the probe by different inline context. then each group work for one context inlining. is my understanding correct?

That's right. Probe in a function profile can have multiple copies due to code duplication. We just want to share the corresponding samples among these duplications, but not among all probes with same id but from different function profiles.

I see, thanks for the helpful explanation.

hoy updated this revision to Diff 345590.May 14 2021, 5:28 PM

Addressing Wei's comment.

wmi accepted this revision.May 14 2021, 6:30 PM

LGTM.

This revision is now accepted and ready to land.May 14 2021, 6:30 PM
wenlei accepted this revision.May 15 2021, 5:40 PM

lgtm, with some nit on comments.

llvm/include/llvm/Transforms/IPO/SampleProfileProbe.h
36–39

comment for the key, value details would be helpful

llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
414

Not directly related to this change, but PseudoProbeUpdatePass can use some header comments describing the high level idea for distribution factor update.

The fields in IR/PseudoProbe.h/PseudoProbe can also use some comments.

hoy added inline comments.May 16 2021, 9:50 PM
llvm/include/llvm/Transforms/IPO/SampleProfileProbe.h
36–39

Comment added.

llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
414

Good point. Comments added in both places.

hoy updated this revision to Diff 345746.May 16 2021, 9:51 PM

Addressing Wenle's comments.