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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2117
 
+void SampleProfileMatcher::populateIRLocations(
+    const Function &F, const FunctionSamples &FS,
----------------
nit on function naming: it not only populates `IRLocations`, but also `MatchedCallsiteLocs`.


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


================
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;
----------------
nit: "base inline" is not a canonical term and can be confusing. You mean top-level inliner frame? 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2152
+
+      // TODO: Support line-number based location(AutoFDO).
+      if (FunctionSamples::ProfileIsProbeBased && isa<PseudoProbeInst>(&I)) {
----------------
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.  


================
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());
+      }
----------------
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

  }
}
```


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2158
+
+      if (!isa<CallBase>(&I) || isa<IntrinsicInst>(&I))
+        continue;
----------------
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
```


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2377
   // The value of the map is the name of direct callsite and use empty StringRef
   // for non-direct-call site.
   std::map<LineLocation, StringRef> IRLocations;
----------------
As I read the code I noticed that "non-direct-call site" can be confusing as it can mean call site that is not direct call. But we really mean to say is 1) not a call site, 2) indirect call site? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156722