[PATCH] D156722: [CSSPGO] Support stale profile matching for LTO

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2117
 
+void SampleProfileMatcher::populateIRLocations(
+    const Function &F, const FunctionSamples &FS,
----------------
wenlei wrote:
> wenlei wrote:
> > nit on function naming: it not only populates `IRLocations`, but also `MatchedCallsiteLocs`.
> More like `findAnchorLocations`?
Sounds good to move `MatchedCallsiteLocs ` out of `populateIRLocations`, I also saw you mentioned to break the functions in other patch, I will do the refactoring in a new diff.

Regarding the name `findAnchorLocations`, we use the term "Anchor" for both IR and Profile, see in the `runStaleProfileMatching`, we use the `ProfileAnchors`. So maybe still keep `populateIRlocations` or `findIRLocations`? after the refactoring, it should only be related with IR.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2134
+            // is enough.
+            if (std::optional<PseudoProbe> Probe = extractProbe(I)) {
+              // Find the base inline frame.
----------------
wenlei wrote:
> Given we tested `isa<PseudoProbeInst>(&I)` above, is `extractProbe` ever going to fail? Remove the `if` check or replace it with an assertion? 
Sounds good!


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2135
+            if (std::optional<PseudoProbe> Probe = extractProbe(I)) {
+              // Find the base inline frame.
+              const DILocation *PrevDIL = DIL;
----------------
wenlei wrote:
> nit: "base inline" is not a canonical term and can be confusing. You mean top-level inliner frame? 
changed to top-level frame.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2152
+
+      // TODO: Support line-number based location(AutoFDO).
+      if (FunctionSamples::ProfileIsProbeBased && isa<PseudoProbeInst>(&I)) {
----------------
wenlei wrote:
> nit: instead of spraying TODOs for line-number-based AutoFDO, we can have a top-level top for AutoFDO, and also explain 1) the current behavior for AutoFDO, 2) what actually needs to be done for AutoFDO.  
Thanks for the suggestion, refine the comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2153-2156
+      if (FunctionSamples::ProfileIsProbeBased && isa<PseudoProbeInst>(&I)) {
+        if (std::optional<PseudoProbe> Probe = extractProbe(I))
+          IRLocations.emplace(LineLocation(Probe->Id, 0), StringRef());
+      }
----------------
wenlei wrote:
> For readability, would it make sense to tweak the code a bit so it's explicit that we're handling not-inlined probes.  Initially I got confused since it didn't pay attention to the continue above..
> 
> ```
> if (isa<PseudoProbeInst>(&I)) {
>   get DIL to check for inlining..
>   if (Inlined case)... 
> 
>   else { // not inlined case
> 
>   }
> }
> ```
Sounds good!


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2158
+
+      if (!isa<CallBase>(&I) || isa<IntrinsicInst>(&I))
+        continue;
----------------
wenlei wrote:
> I know this is old code, but what intrinsics are we skipping here, is that just for skipping probe intrinsics or something else? If it's for skipping probes, maybe we can consider this?
> 
> ```
> if (probe inst) {
>   record empty string in IRLocations
> else if (call inst) {
>   record actual call site in IRLocations
> }
> ```
> 
> or 
> ```
> if (probe inst) {
>   record empty string in IRLocations
> else if (not call inst) {
>   continue
> }
> record actual call site in IRLocations
> ```
 `IntrinsicInst` may not only be used for probe intrinsics. I can't remember clearly, but I vaguely remembered there were a lot of dbg intrinsics that messed up the matching, and the fix was to add this `isa<IntrinsicInst>(&I)` check. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156722/new/

https://reviews.llvm.org/D156722