This is an archive of the discontinued LLVM Phabricator instance.

[CSSPGO] Trim cold base profiles for the CS preinliner.
ClosedPublic

Authored by hoy on Oct 25 2021, 2:10 PM.

Details

Summary

Adding support to the CS preinliner to trim cold base profiles. This makes trimming consistent with the inline decision made by the preinliner. Also disable the existing profile merger when preinliner is on unless explicitly specified.

Diff Detail

Event Timeline

hoy created this revision.Oct 25 2021, 2:10 PM
hoy requested review of this revision.Oct 25 2021, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2021, 2:10 PM
hoy updated this revision to Diff 382124.Oct 25 2021, 2:14 PM

Updating D112489: [CSSPGO] Trim cold profiles with the CS preinliner.

wenlei added inline comments.Oct 25 2021, 5:38 PM
llvm/tools/llvm-profgen/CSPreInliner.cpp
286

I think it's cleaner if we defer all trimming to SampleContextTrimmer. We can add TrimBaseProfileOnly flag to trimAndMergeColdContextProfiles interface, or have a separate interface (will still need to make sure we reuse code). We create SampleContextTrimmer right above anyways.

With this change, CSProfMergeColdContext will always be ignored when preinliner is on, but that's not the case before. So alternatively, if TrimBaseProfileOnly is added to the interface, we can call the trimmer outside of preinliner like before, but set TrimBaseProfileOnly to true when preinliner is on.

287

This logging is a bit inconsistent - we don't have logging for this level of activity from SampleContextTrimmer. However if we unify as suggested above, we won't have such inconsistency. I'm not sure if we need such logging though.

hoy added inline comments.Oct 25 2021, 5:59 PM
llvm/tools/llvm-profgen/CSPreInliner.cpp
286

Sounds good. I'll defer SampleContextTrimmer to do the trimming.

There is a gray area with previous implementation when the preinliner and the trim are both on. In that case the previous check below will accidentally cause the profile merger to run. The profile merger defaulted to on but did not actually run when preilniner was on and neither merger/trimmer switch was given. Turning on the trimmer via the switch caused the merger to run which otherwise would not run.

if (!EnableCSPreInliner || CSProfTrimColdContext.getNumOccurrences() ||
      CSProfMergeColdContext.getNumOccurrences())

I think we want to run the trimmer based on the command line switch, regardless of whether preilniner is on. How about the merger?

wenlei added inline comments.Oct 25 2021, 10:47 PM
llvm/tools/llvm-profgen/CSPreInliner.cpp
286

Maybe we should do this below. That makes sure we don't accidentally run the merging if only trimming is asked for. That is - make preinliner and merging either-or unless explicitly specified, but leave trimming orthogonal.

if (EnableCSPreInliner && !CSProfMergeColdContext.getNumOccurrences())
   CSProfMergeColdContext = false;
hoy added inline comments.Oct 25 2021, 11:08 PM
llvm/tools/llvm-profgen/CSPreInliner.cpp
286

That should work. I was thinking if we should completely disable separate merging for preinliner, since we can tune the hotness cutoff to have preinliner do more merge.

hoy updated this revision to Diff 382200.Oct 25 2021, 11:41 PM

Deferring to SampleContextTrimmer to trim cold profiles..

wenlei added inline comments.Oct 27 2021, 4:03 PM
llvm/tools/llvm-profgen/CSPreInliner.cpp
275–283

Sorry if I wasn't clear.

So alternatively, if TrimBaseProfileOnly is added to the interface, we can call the trimmer outside of preinliner like before, but set TrimBaseProfileOnly to true when preinliner is on.

What I meant by the above to do the trimming outside of preinliner. So preinliner does strictly preinlining, and trimming is done at the original place, with TrimBaseProfileOnly set to EnableCSPreInliner. This way we don't have trimming functionality scattered.

The setting of CSProfMergeColdContext also better be done outside of preinliner.

286

Yeah, that would be the default behavior. But if user asks for merging on top of preinliner, it rarely makes sense but I assume they know what they're doing.

hoy added inline comments.Oct 27 2021, 4:47 PM
llvm/tools/llvm-profgen/CSPreInliner.cpp
275–283

I found that separating the logic for the preinlining and non-preinlining path is easier to read and less error prone. Don't have a strong preference. Can do what you suggested.

hoy updated this revision to Diff 382858.Oct 27 2021, 5:00 PM

Addressing Wenlei's comment.

llvm/tools/llvm-profgen/CSPreInliner.cpp
275–283

Actually the logic looks better after moving this out.

wlei added inline comments.Oct 27 2021, 5:23 PM
llvm/lib/ProfileData/SampleProf.cpp
335

what if TrimBaseProfileOnly is true but those two are false, shall we skip the trim?

maybe change to if (!TrimColdContext && !MergeColdContext && !TrimBaseProfileOnly)

357

if(MergeColdContext && !TrimBaseProfileOnly)

So that MergedProfileMap will be empty for TrimBaseProfileOnly

367

Then we can add an assertion: TrimBaseProfileOnly && MergedProfileMap.empty()

hoy added inline comments.Oct 27 2021, 5:27 PM
llvm/lib/ProfileData/SampleProf.cpp
335

TrimBaseProfileOnly should only be effective when TrimColdContext is true. Trimming means to trim cold profile. On top of that we can choose to trim all cold profiles or only base cold profiles. Does that make sense? I can add a comment for that.

wenlei accepted this revision.Oct 27 2021, 5:35 PM

lgtm, thanks.

llvm/lib/ProfileData/SampleProf.cpp
335

Yeah, sounds good to me. A comment would be helpful.

llvm/tools/llvm-profgen/CSPreInliner.cpp
275–283

Yes, I think separating them out is cleaner.

This revision is now accepted and ready to land.Oct 27 2021, 5:35 PM
wlei added inline comments.Oct 27 2021, 5:46 PM
llvm/lib/ProfileData/SampleProf.cpp
335

Makes sense.

I was thinking if we could reuse this for non-CS cold profile trimming(we have that in our internal patch). in that case, it's the same logic as TrimBaseProfileOnly = true. Anyway, that should be another patch. thanks!

hoy updated this revision to Diff 382870.Oct 27 2021, 6:04 PM

Adding comment to trimAndMergeColdContextProfiles

hoy retitled this revision from [CSSPGO] Trim cold profiles with the CS preinliner. to [CSSPGO] Trim cold base profiles for the CS preinliner..Oct 27 2021, 6:05 PM
hoy edited the summary of this revision. (Show Details)
wlei added inline comments.Oct 27 2021, 6:13 PM
llvm/lib/ProfileData/SampleProf.cpp
357

How about this comments? is MergeColdContext always false when TrimBaseProfileOnly is true?

hoy added inline comments.Oct 27 2021, 6:56 PM
llvm/lib/ProfileData/SampleProf.cpp
357

Sorry for overlooking this comment. MergeColdContext and TrimBaseProfileOnly can be both true when user specifies the switches for both merging and trimming. We were talking about whether to turn off merging for preinliner completely and we agree to let user decide. In that case, I think we should ignore TrimBaseProfileOnly since merging is meant to not honor the preinliner decision.

It is a good catch, thanks!

hoy updated this revision to Diff 382882.Oct 27 2021, 7:02 PM

Addressing Lei's comment.

wlei accepted this revision.Oct 27 2021, 7:08 PM

LGTM.

llvm/lib/ProfileData/SampleProf.cpp
357

Thanks for the clarification!

This revision was landed with ongoing or failed builds.Oct 27 2021, 10:50 PM
This revision was automatically updated to reflect the committed changes.