[PATCH] D92347: [CSSPGO] Consume pseudo-probe-based AutoFDO profile

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:554-556
+      // A missing entry for a probe likely means the probe was not executed.
+      // Treat it as a zero count instead of an unknown count to help edge
+      // weight inference.
----------------
hoy wrote:
> wmi wrote:
> > I guess it makes sense to treat missing entry to have zero count instead of unknown because the function profiles with unmatched CFGchecksum has been dropped, is that right? 
> > 
> > If it is, can we check CFGchecksum not zero here instead of ProfileIsProbeBased or ProfileIsCS? When CFGchecksum is not zero and matched, we are confident the profile is accurate no matter what kind of profile it is. 
> > 
> > Another thing is: I think debug info based profile can have a similar checksum then it can use a similar strategy here as probe based profile. It may not be a CFGchecksum because debug info based profile has no idea about CFG, so it is better to rename CFGchecksum to something more general.   
> I think the comment I put here is a bit confusing. A missing entry for a probe here really means a missing counter. When a probe counter doesn't appear in a function profile, we are sure that the probe has not been executed (virtually). From our experiment, giving it a zero count is a bit more accurate instead of having the counts inference algorithm to figure out a count for it.
> 
> We use CFGchecksum to identify if a profile is debug-info-based or pseudo-probe-based. A probe-based profile will likely have a non-zero checksum due to the way CFGchecksum is computed. As you pointed out, when CFGchecksum is matched, we are confident the profile is accurate (even if there are missing counters for some probes). If not matched, the profile will be completely dropped and no counters will be applied to the function..
> 
> It's a good suggestion to apply a similar checksum for debug-info-based profile to tell how close the pervious source is to the current IR. We can discuss more about this.
> 
> 
> 
> A missing entry for a probe here really means a missing counter.

Could you list the possible cases you have known where probe could be missing? 

> From our experiment, giving it a zero count is a bit more accurate instead of having the counts inference algorithm to figure out a count for it.

Yes, I agree with that. I just want to know whether it makes sense to check CFGChecksum match instead of checking the profile is probe based or context sensitive.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92347