- 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
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
llvm/lib/Transforms/IPO/SampleProfile.cpp | ||
---|---|---|
464 | this can actually be a valid c mangle name.. we need something more distinctive.. *UnkownIndirectCallee? | |
2158 | nit: the CalleeName -> CalleeName. | |
2160 | nit: avoid nums of false positive -> reduce num of false positives or avoid false positive | |
2238 | This is passed by value, missing &? | |
2365–2389 | 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? |
looks cleaner and more structured now, thanks for update!
llvm/lib/Transforms/IPO/SampleProfile.cpp | ||
---|---|---|
2377–2384 | nit: move this into runStaleProfileMatching as well? |
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. | |
2158 | done. | |
2160 | done | |
2238 | Good catch, removed populateProfileCallsites | |
2365–2389 | 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.
|
llvm/lib/Transforms/IPO/SampleProfile.cpp | ||
---|---|---|
2365–2389 | Yeah, I think the term Anchor is abstract enough that including non-call is also okay. |
llvm/lib/Transforms/IPO/SampleProfile.cpp | ||
---|---|---|
2128 | 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? |
llvm/lib/Transforms/IPO/SampleProfile.cpp | ||
---|---|---|
2128 | 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. |
llvm/lib/Transforms/IPO/SampleProfile.cpp | ||
---|---|---|
2128 | 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. |
llvm/lib/Transforms/IPO/SampleProfile.cpp | ||
---|---|---|
2128 | 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. |
llvm/lib/Transforms/IPO/SampleProfile.cpp | ||
---|---|---|
2128 |
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 can actually be a valid c mangle name.. we need something more distinctive.. *UnkownIndirectCallee?