[PATCH] [MBP] Fix a really horrible bug in MachineBlockPlacement, but behinda flag for now.

[Daniel Jasper]

Hi chandlerc,

This is similar to Chandler's r231238 (most details on that commit log).

The difference is that Chandler's version led to putting outlined blocks in reverse order, which does seem to have a quite dramatic performance overhead.

In contrast, this version only picks the back of the worklist, if the previously placed machine block had a successor that wasn't placed for other reasons, especially:
* It was cold.
* It had a globally more important predecessor.

Using the back of the worklist in those circumstances keeps outlined branches together. Once an outlined branch was placed completely, there simply won't be any more successors and it is more useful to start with the hottest block from the entire worklist (and the first one for equal probabilities).

Tests TBD.

http://reviews.llvm.org/D8104

Files:
  lib/CodeGen/MachineBlockPlacement.cpp

Index: lib/CodeGen/MachineBlockPlacement.cpp
===================================================================
--- lib/CodeGen/MachineBlockPlacement.cpp
+++ lib/CodeGen/MachineBlockPlacement.cpp
@@ -74,6 +74,13 @@
              "post dominator, out of line."),
     cl::init(false), cl::Hidden);
 
+static cl::opt<bool> PlaceLastSuccessor(
+    "place-last-successor",
+    cl::desc("When selecting a non-successor block, choose the last block to "
+             "have been a successor. This represents the block whose "
+             "predecessor was most recently placed."),
+    cl::init(false), cl::Hidden);
+
 namespace {
 class BlockChain;
 /// \brief Type for our function-wide basic block -> block chain mapping.
@@ -223,11 +230,11 @@
                            const BlockFilterSet *BlockFilter = nullptr);
   MachineBasicBlock *selectBestSuccessor(MachineBasicBlock *BB,
                                          BlockChain &Chain,
-                                         const BlockFilterSet *BlockFilter);
-  MachineBasicBlock *
-  selectBestCandidateBlock(BlockChain &Chain,
-                           SmallVectorImpl<MachineBasicBlock *> &WorkList,
-                           const BlockFilterSet *BlockFilter);
+                                         const BlockFilterSet *BlockFilter,
+                                         bool &HasUnmergedSuccessors);
+  MachineBasicBlock *selectBestCandidateBlock(
+      BlockChain &Chain, SmallVectorImpl<MachineBasicBlock *> &WorkList,
+      const BlockFilterSet *BlockFilter, bool HasUnmergedSuccessors);
   MachineBasicBlock *
   getFirstUnplacedBlock(MachineFunction &F, const BlockChain &PlacedChain,
                         MachineFunction::iterator &PrevUnplacedBlockIt,
@@ -343,7 +350,8 @@
 MachineBasicBlock *
 MachineBlockPlacement::selectBestSuccessor(MachineBasicBlock *BB,
                                            BlockChain &Chain,
-                                           const BlockFilterSet *BlockFilter) {
+                                           const BlockFilterSet *BlockFilter,
+                                           bool &HasUnmergedSuccessors) {
   const BranchProbability HotProb(4, 5); // 80%
 
   MachineBasicBlock *BestSucc = nullptr;
@@ -370,6 +378,8 @@
       continue;
     }
 
+    HasUnmergedSuccessors = true;
+
     uint32_t SuccWeight = MBPI->getEdgeWeight(BB, Succ);
     BranchProbability SuccProb(SuccWeight / WeightScale, SumWeight);
 
@@ -436,7 +446,25 @@
 /// \returns The best block found, or null if none are viable.
 MachineBasicBlock *MachineBlockPlacement::selectBestCandidateBlock(
     BlockChain &Chain, SmallVectorImpl<MachineBasicBlock *> &WorkList,
-    const BlockFilterSet *BlockFilter) {
+    const BlockFilterSet *BlockFilter, bool HasUnmergedSuccessors) {
+  if (PlaceLastSuccessor && HasUnmergedSuccessors) {
+    // If we're just placing the last successor as the best candidate, the
+    // logic is super simple. We skip the already placed entries on the
+    // worklist and return the most recently added entry that isn't placed.
+    while (!WorkList.empty()) {
+      MachineBasicBlock *SuccBB = WorkList.pop_back_val();
+      BlockChain &SuccChain = *BlockToChain.lookup(SuccBB);
+      if (&SuccChain == &Chain) {
+        DEBUG(dbgs() << "    " << getBlockName(SuccBB)
+                     << " -> Already merged!\n");
+        continue;
+      }
+      assert(SuccChain.LoopPredecessors == 0 && "Found CFG-violating block");
+      return SuccBB;
+    }
+
+    return nullptr;
+  }
   // Once we need to walk the worklist looking for a candidate, cleanup the
   // worklist of already placed entries.
   // FIXME: If this shows up on profiles, it could be folded (at the cost of
@@ -513,13 +541,16 @@
 
     // Look for the best viable successor if there is one to place immediately
     // after this block.
-    MachineBasicBlock *BestSucc = selectBestSuccessor(BB, Chain, BlockFilter);
+    bool HasUnmergedSuccessors = false;
+    MachineBasicBlock *BestSucc =
+        selectBestSuccessor(BB, Chain, BlockFilter, HasUnmergedSuccessors);
 
     // If an immediate successor isn't available, look for the best viable
     // block among those we've identified as not violating the loop's CFG at
     // this point. This won't be a fallthrough, but it will increase locality.
     if (!BestSucc)
-      BestSucc = selectBestCandidateBlock(Chain, BlockWorkList, BlockFilter);
+      BestSucc = selectBestCandidateBlock(Chain, BlockWorkList, BlockFilter,
+                                          HasUnmergedSuccessors);
 
     if (!BestSucc) {
       BestSucc =

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D8104.21347.patch
Type: text/x-patch
Size: 4619 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150306/f2552e45/attachment.bin>