This is an archive of the discontinued LLVM Phabricator instance.

[CSSPGO] Refactoring SampleProfileMatcher::runOnFunction
ClosedPublic

Authored by wlei on Aug 24 2023, 11:32 PM.

Details

Summary
  • rename IRLocation --> IRAnchors, ProfileLocation --> ProfileAnchors
  • reorganize runOnFunction, fact out the finding IR anchors code into findIRAnchors
  • introduce a new function findProfileAnchors to populate the profile related anchors, the result is saved into ProfileAnchors, it's later used for both mismatch report and matching, this can avoid to parse the getBodySamples and getCallsiteSamples for multiple times.
  • move the MatchedCallsiteLocs stuffs from findIRAnchors to countProfileMismatches so that all the staleness metrics report are computed in one function.
  • move all matching related into runStaleProfileMatching, and move all mismatching report into countProfileMismatches

Diff Detail

Event Timeline

wlei created this revision.Aug 24 2023, 11:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2023, 11:32 PM
wlei requested review of this revision.Aug 24 2023, 11:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2023, 11:32 PM
wlei edited the summary of this revision. (Show Details)Aug 24 2023, 11:42 PM
wlei added reviewers: hoy, wenlei.
wlei added inline comments.Aug 24 2023, 11:46 PM
llvm/test/Transforms/SampleProfile/profile-mismatch.ll
9

This is due to I changed to use the totalSamples instead of headerSamples for the mismatched inlined samples, since we switched to use the original nested profile, using totalSamples should makes more sense.

wenlei added inline comments.Aug 25 2023, 10:20 AM
llvm/lib/Transforms/IPO/SampleProfile.cpp
464

this can actually be a valid c mangle name.. we need something more distinctive.. *UnkownIndirectCallee?

2214

nit: the CalleeName -> CalleeName.

2216

nit: avoid nums of false positive ->

reduce num of false positives

or

avoid false positive

2263

This is passed by value, missing &?

2379–2416

Trying to organize the code a bit and keep high level flow clean, how about this:

// ..
std::map<LineLocation, StringRef> IRAnchors;
findIRAnchors(FS, IRAnchors);

// ..
std::map<LineLocation, StringSet<>> ProfileAnchors;
findProfileAnchors(FS, ProfileLocations);

// Detect profile mismatch for profile staleness metrics report.
if (ReportProfileStaleness || PersistProfileStaleness) {
  countProfileMismatches(FS, IRAnchors, ProfileAnchors, ...)
}

if (IsFuncHashMismatch && SalvageStaleProfile) {
   runStaleProfileMatching(IRAnchors, ProfileAnchors, getIRToProfileLocationMap(F));
}

Accordingly, we would need to move populateProfileCallsites in runStaleProfileMatching. Maybe even inline expand runStaleProfileMatching there without a function call since the function is now simplified? Also move all statistics into countProfileMismatches.

Inside runStaleProfileMatching, we'd also rename ProfileAnchors to MatchedAnchors.

llvm/test/Transforms/SampleProfile/profile-mismatch.ll
27

what causes this change?

wlei updated this revision to Diff 553589.Aug 25 2023, 1:11 PM

addressing feedback

looks cleaner and more structured now, thanks for update!

llvm/lib/Transforms/IPO/SampleProfile.cpp
2404–2409

nit: move this into runStaleProfileMatching as well?

wlei added inline comments.Aug 25 2023, 1:22 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
464

Good catch, changed use "dot" as the separate char, that should not be used as c/c++ function name.

2214

done.

2216

done

2263

Good catch, removed populateProfileCallsites

2379–2416

That would looks much cleaner, thanks for the suggestion! done accordingly.

Only one things is that the name IRAnchors, it is not only the Anchors, we also need the non-call locations whose callee name is empty. but I understand separating the non-call locations and the anchors doesn't look neat. I don't have strong option on this, maybe this is straightforward to understand.

llvm/test/Transforms/SampleProfile/profile-mismatch.ll
27

this is the same reason as the line 9.

"I changed to use the totalSamples instead of headerSamples for the mismatched inlined samples, since we switched to use the original nested profile, using totalSamples should makes more sense."

wenlei added inline comments.Aug 25 2023, 1:29 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2379–2416

Yeah, I think the term Anchor is abstract enough that including non-call is also okay.

wlei updated this revision to Diff 553595.Aug 25 2023, 1:30 PM

move dbg print into runStaleProfileMatching

wenlei accepted this revision.Aug 25 2023, 1:31 PM

lgtm, thanks.

This revision is now accepted and ready to land.Aug 25 2023, 1:31 PM
wlei retitled this revision from [CSSPGO] Refactoring populateIRLocations and countProfileMismatches to [CSSPGO] Refactoring SampleProfileMatcher::runOnFunction.Aug 25 2023, 1:45 PM
wlei edited the summary of this revision. (Show Details)
wlei updated this revision to Diff 553606.Aug 25 2023, 1:52 PM
wlei edited the summary of this revision. (Show Details)

fix typo

hoy added inline comments.Aug 29 2023, 4:57 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2193

extractProbe can also be used to extract a probe from a callsite. Can this code be unified with the callsite handling code below to avoid the overwrite?

wlei added inline comments.Aug 29 2023, 5:46 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2193

Try to understand this, do you mean we can just use the LineLocation(Probe->Id, 0) as the callsite location, it then can replace the LineLocation IRCallsite = FunctionSamples::getCallSiteIdentifier(DLoc);?

If so, that's a good idea for pseudo-probe.

However, currently callsite handling part of the code are still used for AutoFDO, we use the calliste related staleness metrics for AutoFDO. Then we probably still need the callsite handling code.

hoy added inline comments.Aug 29 2023, 8:57 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2193

I think you can just do this

if (FunctionSamples::ProfileIsProbeBased) {
     if (auto Probe = extractProbe(I)) {
         StringRef CalleeName;
         if (const auto *CB = dyn_cast<CallBase>(&I)) {
             if (Function *Callee = I.getCalledFunction()){
                 CalleeName = FunctionSamples::getCanonicalFnName(Callee->getName());
            } else {
               CalleeName = UnknownIndirectCallee;
            }
         }
         IRAnchors.emplace(LineLocation(Probe->Id, 0), CalleeName);
    }
}

Let me know if it works. As for AutoFDO, we could probably leave it a TODO just as what you do now and redesign the whole logic later.

wlei added inline comments.Aug 29 2023, 9:08 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2193

Thanks for the suggestion. That looks cleaner, let me give it a try.

I guess we even don't need the FunctionSamples::ProfileIsProbeBased.

Also do you mind I try it in a new patch, on top of the stack, because this patch is the bottom of the patch stack, then I need to more rebasing and then need to request for review for the accepted patch.

hoy added inline comments.Aug 29 2023, 9:17 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2193

I guess we even don't need the FunctionSamples::ProfileIsProbeBased.

It should still be useful. In theory, we can still use AutoFDO profile for a probed function. Actually our dual-profile mode is just a use case.

Sounds good to make it a new patch.

This revision was automatically updated to reflect the committed changes.