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

Tue Dec 15 18:41:58 PST 2020

hoy 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:
> > wmi wrote:
> > > wmi wrote:
> > > > hoy wrote:
> > > > > hoy wrote:
> > > > > > wmi wrote:
> > > > > > > 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.  
> > > > > > Yeah. A probe is deleted literally only if the corresponding block is really dead. Otherwise if the probe counter is not sampled, it means the block is there but just was not run. We trust this and assign a zero weight to the probe. There is a third case where a probe is sampled through its adjacent physical instruction but the samples are not trusted because the next physical instruction starts a new block. We call that case a "dangling" probe. This usually reflects that all instructions of a block are moved out of that block and only that probe is left there. For dangling probes we intentionally record a zero count in the profile instead of a missing counter. This helps the counts inference algorithm. Will have a patch to deal with optimizations that result in dangling probes.
> > > > > > 
> > > > > > I see. So far the check against probe-based is equivalent to checking against CFGChecksum, since that's the way to identify if a profile is probe-based or debug-info based. If we'd like to also employ a CFGChecksum for the debug-info scenario in future, it's reasonable to check CFGChecksum here.
> > > > > Given a second thought, I'm not sure how we'd like to make a CFGChecksum for the debug-info profile. Maybe just check ProfileIsProbeBased for now and change it to checking CFGChecksum when it comes to extending debug-info profile? What do you think?
> > > > Is there case that optimizations after probe insertion create new BBs and the new BBs somehow don't have probes?
> > > > 
> > > > > So far the check against probe-based is equivalent to checking against CFGChecksum, since that's the way to identify if a profile is probe-based or debug-info based.
> > > > 
> > > > Ok, it is fine to leave the change later. 
> > > Yes, I agree with it. It is fine to adjust it later. 
> > > Is there case that optimizations after probe insertion create new BBs and the new BBs somehow don't have probes?
> > 
> > I still curious about the answer to the question above. Could you share your thought? 
> Sorry for missing the comment. 
> 
> That's a good question. Yes, there will be blocks created after probe insertion that won't get a probe. I hope that's not too common if the new blocks really contain code from the original IR. And if that happens, we could rely on the counts inference pass to mitigate that. The worst case is that optimizer creates a conditional structure with none of the new blocks copied from the original IR and places some user code in them. Since there's no probe in the new blocks, we won't know anything about how frequent that code is executed during sampling. I guess in that case we would need to fix the optimizer to copy the original probe along with the code, if the optimization is not done on block level.
I could write a pass to validate that every block with real instructions gets a probe and run it after each optimization. That should give us a view of how common the missing probe cases are. Then we can start fixing the offending passes from there. What do you think? Thanks for raising the issue.  

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92347