[PATCH] D152399: [CodeGen] Fine tune MachineFunctionSplitPass (MFS) for FSAFDO.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:182-188
+  if (!CandidateForSplit) {
+    // Split all EH code and it's descendant statically by default.
+    if (SplitAllEHCode)
+      setDescendantEHBlocksCold(MF);
+    finishAdjustingBasicBlocksAndLandingPads(MF);
+    return true;
   }
----------------
Does this change the behavior for UseProfileData == false, even if HasAccurateProfile == true? 

Can we move this up to line 177, and do the following directly?

```
if (UseProfileData) {
  MBFI = &getAnalysis<MachineBlockFrequencyInfo>();
  PSI = &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
  // Comments ..
  if (!HasAccurateProfile && !PSI->isFunctionHotInCallGraph(&MF.getFunction(), MBFI)) {
    // Split all EH code and it's descendant statically by default.
    if (SplitAllEHCode)
      setDescendantEHBlocksCold(MF);
    finishAdjustingBasicBlocksAndLandingPads(MF);
    return true;
  }
}
```




================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:165
+          new BranchProbabilityInfo(MF.getFunction(), LI);
+      BlockFrequencyInfo BFI(MF.getFunction(), *BPI, LI);
+      CandidateForSplit =
----------------
shenhan wrote:
> wenlei wrote:
> > We don't need to go through BPI->BFI all over again just to use `PSI->isFunctionHotInCallGraph`, which is more expensive. Instead, we can implement this directly with MBFI. 
> > 
> > MachineSizeOpts has the same need and has implemented `machine_size_opts_detail::isFunctionColdInCallGraph`, `machine_size_opts_detail::isFunctionHotInCallGraphNthPercentile` already. Now that we have a second use, it makes sense to refactor these into common MIR hotness helpers. 
> This is being addressed in D152758. I'll keep it as is for now (after addressing Snehasish's comments). Once D152758 is submitted, I'll change this accordingly.
> 
> 
> Once D152758 is submitted, I'll change this accordingly. 

Since you stacked the patches already, you can make changes here already. It will make code cleaner and review easier.  


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:111
+  if (IsFSAFDOFlavor)
+    return Count.has_value() && Count.value() == 0;
+
----------------
shenhan wrote:
> wenlei wrote:
> > shenhan wrote:
> > > shenhan wrote:
> > > > wenlei wrote:
> > > > > snehasish wrote:
> > > > > > Can we use `< ColdCountThreshold` instead of `== 0`?
> > > > > > 
> > > > > > `ColdCountThreshold` defaults to 1 so it should have the same effect.
> > > > > So for IRPGO, we want to treat unknown as zero/cold, but not so for FSAFDO? Why is the difference? 
> > > > > 
> > > > > IIRC after profile inference, not sampled block will still get zero counts. What produces `Count.has_value() == false`?
> > > > > So for IRPGO, we want to treat unknown as zero/cold, but not so for FSAFDO? Why is the difference?
> > > > 
> > > > Yes, the reason is that when IRPGO dictates a zero block counter, we trust it with higher confidence, but less so for FSAFDO.  The latter could be a miss sampling or inference / propagation error.  Since the cost of splitting a non-cold block is high, we only split when we are sure of the block counter.
> > > > 
> > > > > IIRC after profile inference, not sampled block will still get zero counts. What produces Count.has_value() == false?
> > > > You are right. This is an extra caution in (unlikely) case when Count has no value and we don't want an exception to be thrown.
> > > Done.
> > > Yes, the reason is that when IRPGO dictates a zero block counter, we trust it with higher confidence, but less so for FSAFDO. The latter could be a miss sampling or inference / propagation error.  
> > 
> > But checking `has_value` isn't going to be able to differentiate inaccurate counts. 
> > 
> > > You are right. This is an extra caution in (unlikely) case when Count has no value and we don't want an exception to be thrown. 
> > 
> > In this case, we don't need to guard it under `HasAccurateProfile` as it applies to IRPGO as well.
> Thanks. I made the following change:
> 
> Firstly, per hoy's comment, I replaced "if (HasAccurateProfile) {" with "if (!HasAccurateProfile) {", noticing that HasAccurateProfile should be False when using FSAFDO or CSSPGO, and True when using IRPGO. Previously the value is negated.
> 
> Secondly, when !HasAccurateProfile && !Count.has_value()  (although the latter is unlikely), this needs to return false. Whereas when HasAccurateProfile  && !Count.has_value(), this returns true. So I have to guard it with HasAccureateProfile.
> 
> Thirdly, the next line "if (!Count)" is not testing "if (Count !=0)", it is testing "if(!Count.has_value())", it is little bit misleading, so I changed it to if(!Count.has_value()), so it is clear why this is guarded under HasAccurateProfile.
> 
> Hope this is clearer.
If the key difference is how we treat unknown/missing counts, I'd suggest let's make it explicit.

Would it work if we just make a simple and explicit change:

```
if (!Count)
  return true;
```
-->
```
// Treat unknown/missing counts as cold if profile is accurate, but not if profile is inaccurate. 
if (!Count.has_value()) {
  return HasAccurateProfile;
}
```


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

https://reviews.llvm.org/D152399