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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added a comment.

> I'll remove the testcase from this CL

Actually I would prefer the foo4 based test case in this patch and in the same file too (`test/CodeGen/X86/machine-function-splitter.ll`). Also added a comment in the other patch D152577 <https://reviews.llvm.org/D152577>. Thanks!



================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:155
   ProfileSummaryInfo *PSI = nullptr;
+  bool candidate_for_split = true;
   if (UseProfileData) {
----------------
shenhan wrote:
> snehasish wrote:
> > 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?
> > This should be CandidateForSplit.
> 
> Ah, yes.
> 
> > Then you can remove the use of this var from the pass altogether?
> Maybe no, because this pass later also splits ALL eh code later.
> 
> 
> ```
> if (SplitAllEHCode)
>     setDescendantEHBlocksCold(MF);
> ```
Hmm, I wonder if we can restructure this to simplify the conditions. I'll respond here if I can think of something.


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:162
+      const BranchProbabilityInfo *BPI =
+          new BranchProbabilityInfo(MF.getFunction(), *LI);
+      BlockFrequencyInfo *BFI =
----------------
shenhan wrote:
> snehasish wrote:
> > 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);
> > ```
> Done.
Change BranchProbabilityInfo on L163 too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152399