[PATCH] D73387: [MBP] Partial tail duplication into hot predecessors

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3034
+      return false;
+    if (CandidatePreds.size() < BB->pred_size())
+      CandidatePtr = &CandidatePreds;
----------------
xur wrote:
> It seems to me that findDuplicateCandidates() will not let CandidatePreds to be full Preds (it will put the first one to fallthrough -- which makes this condition always true).
Only when Candidates.size() < Preds.size() is true then it changes the first predecessor to fall through.

If Candidates.size() == Preds.size(), the first predecessor will not be changed to fall through.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3071
+
+// Returns true if BB is Pred's most possible successor.
+bool MachineBlockPlacement::isBestSuccessor(MachineBasicBlock *BB,
----------------
xur wrote:
> what does "most possible" mean here?
It should be changed to "best".


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3106
+
+void MachineBlockPlacement::findDuplicateCandidates(
+    SmallVectorImpl<MachineBasicBlock *> &Candidates,
----------------
xur wrote:
> davidxl wrote:
> > Add some description (and examples) showing the algorithm/cost model used here.
> As David said, need to have cost model description here.
> 
> I give my first impression here:
> (1) should you check if BB can be placed after a Pred? Pred could be in the middle of the chain.
> (2) It seems that you assume the Succ BBs will be chosen as fall-through by different Dupped BBs.
>  Should you check if SuccIt has already be placed? Also, if SuccIt can be duplicated, the assumption does not to hold.
> (3) Pred fall-through can be handled better. this algorithm does not actually try to find the optimal fall through of BB. It passively get the fall-through when we cannot duplicate. Maybe we can refactor code better so the post fixup at line 3171 is not needed.
If BB can't be duplicated into Pred, isBestSuccessor checks if Pred is in the middle of a chain.
If BB can be duplicated into Pred, Pred has the only successor BB, it is extremely unlikely in the middle of a chain.

The main purpose of the loop is finding out beneficial duplications. If a predecessor has multiple successors we can't duplicate BB into it, even if the edge is hot. But we can make it fall through to BB to get the same benefit as tail duplication.
Change one duplication into fall through is only a plus for code size.



================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3159
+
+    if (DupCost >= OrigCost)
+      continue;
----------------
xur wrote:
> I don't think this will ever be true.
Good catch.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3191
+
+  // FIXME: we may use profile count instead of frequency,
+  // and need more fine tuning.
----------------
xur wrote:
> I think using profile count will get you the same thing here. Profile count is derived from frequency. 
I mean compare the raw profile count with the hot number from profile summary information.


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

https://reviews.llvm.org/D73387