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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:111
+  if (IsFSAFDOFlavor)
+    return Count.has_value() && Count.value() == 0;
+
----------------
Can we use `< ColdCountThreshold` instead of `== 0`?

`ColdCountThreshold` defaults to 1 so it should have the same effect.


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:155
   ProfileSummaryInfo *PSI = nullptr;
+  bool candidate_for_split = true;
   if (UseProfileData) {
----------------
This should be `CandidateForSplit`.

Also since if CandidateForSplit is false, we can just return early from L166 since we are not going to split anything in this function. Then you can remove the use of this var from the pass altogether?


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:160
+    if (IsFSAFDOFlavor) {
+      const LoopInfo *LI = &getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
+      const BranchProbabilityInfo *BPI =
----------------
`const LoopInfo& LI`?
No need to take the address and then deref it in each use below.


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:162
+      const BranchProbabilityInfo *BPI =
+          new BranchProbabilityInfo(MF.getFunction(), *LI);
+      BlockFrequencyInfo *BFI =
----------------
I think this new (and the one below) is leaking memory. 

Maybe just use vars on the stack like --
```
const BranchProbabilityInfo BPI(MF.getFunction(), LI);
BlockFrequencyInfo BFI(MF.getFunction(), BPI, LI);
```


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1253
 
+  if (EnableFSDiscriminator)
+    addPass(createMIRAddFSDiscriminatorsPass(
----------------
I think the changes to TargetPassConfig should be split out into a separate patch. Also needs a test to ensure that the additional discriminator passes are run where we expect them to be run.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152399