Factor out MachineBlockPlacement::fillWorkLists. NFC

[deadalnix]

Summary

There are places in MachineBlockPlacement where a worklist is filled in pretty much identical way. The code is duplicated. This refactor it so that the same code is used in both scenarii.

[mcrosier]

lib/CodeGen/MachineBlockPlacement.cpp

591 ↗

(On Diff #50395)

Should this be a continue?

593 ↗

(On Diff #50395)

Same as above. continue?

598 ↗

(On Diff #50395)

If you don't mind, I'd prefer to stick with the style of the original code.

if (Chain.UnscheduledPredecessors == 0)
  BlockWorkList.push_back(*Chain.begin());

[deadalnix]

lib/CodeGen/MachineBlockPlacement.cpp

593 ↗

(On Diff #50395)

You are right. How come this didn't broke any tests ? It seems like a pretty bad breakage.

598 ↗

(On Diff #50395)

I changed it because of D17625 (which this diff is extracted from).

[deadalnix]

Use continue where apropriate.

[mcrosier]

lib/CodeGen/MachineBlockPlacement.cpp

593 ↗

(On Diff #50462)

Perhaps it would be a good idea to add such a test case.

598 ↗

(On Diff #50462)

Was this style suggested in D17625?

[deadalnix]

lib/CodeGen/MachineBlockPlacement.cpp

598 ↗

(On Diff #50462)

D17625 introduce 2 worklist, so you need to add code there to choose which worklist you add the MBB to.

[mcrosier]

lib/CodeGen/MachineBlockPlacement.cpp

598 ↗

(On Diff #50462)

Honestly, I don't follow your argument.

I prefer this style (i.e., the original code):

if (Chain.UnscheduledPredecessors == 0)
  BlockWorkList.push_back(*Chain.begin());

rather than your change (below):

if (Chain.UnscheduledPredecessors != 0)
  return;
BlockWorkList.push_back(*Chain.begin());

For this patch can we just stick with the original code?

[deadalnix]

lib/CodeGen/MachineBlockPlacement.cpp

593 ↗

(On Diff #50462)

Do you have a suggestion on how to build such test case ?

598 ↗

(On Diff #50462)

It can. I just would have to change this in D17625 instead of here, which really I don't care. I was asked to extract NFC changes in this patch, so that made sense to include this.

[davidxl]

lib/CodeGen/MachineBlockPlacement.cpp

598 ↗

(On Diff #50462)

I agree with Chad here -- unless it helps reducing if nesting level, early return here does not help readability -- the original code seems more readable:

if (Chain.UnscheduledPredecessors == 0) {

// update worklist(s)

}
return;

[deadalnix]

Style line 598

[davidxl]

lib/CodeGen/MachineBlockPlacement.cpp

277 ↗

(On Diff #50515)

Though not done for existing methods, I think it is better to add a slightly detailed comments for this method. Otherwise the refactoring looks good.

llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp

class MachineBlockPlacement : public MachineFunctionPass {
   MachineBasicBlock *
   selectBestCandidateBlock(BlockChain &Chain,
                            SmallVectorImpl<MachineBasicBlock *> &WorkList,
                            const BlockFilterSet *BlockFilter);
   MachineBasicBlock *
   getFirstUnplacedBlock(MachineFunction &F, const BlockChain &PlacedChain,
                         MachineFunction::iterator &PrevUnplacedBlockIt,
                         const BlockFilterSet *BlockFilter);
+
+  /// \brief Add a basic block to the work list if it is apropriate.
+  ///
+  /// If the optional parameter BlockFilter is provided, only MBB
+  /// present in the set will be added to the worklist. If nullptr
+  /// is provided, no filtering occurs.
+  void fillWorkLists(MachineBasicBlock *MBB,
+                     SmallPtrSetImpl<BlockChain *> &UpdatedPreds,
+                     SmallVectorImpl<MachineBasicBlock *> &BlockWorkList,
+                     const BlockFilterSet *BlockFilter);
   void buildChain(MachineBasicBlock *BB, BlockChain &Chain,
                   SmallVectorImpl<MachineBasicBlock *> &BlockWorkList,
                   const BlockFilterSet *BlockFilter = nullptr);
   MachineBasicBlock *findBestLoopTop(MachineLoop &L,
                                      const BlockFilterSet &LoopBlockSet);
   MachineBasicBlock *findBestLoopExit(MachineFunction &F, MachineLoop &L,
                                       const BlockFilterSet &LoopBlockSet);
   BlockFilterSet collectLoopBlockSet(MachineFunction &F, MachineLoop &L);
if (BlockToChain[&*I] != &PlacedChain) {
       // as the block to place. This will force the entire chain to be placed,
       // and satisfies the requirements of merging chains.
       return *BlockToChain[&*I]->begin();
     }
   }
   return nullptr;
 }
 
+void MachineBlockPlacement::fillWorkLists(
+    MachineBasicBlock *MBB,
+    SmallPtrSetImpl<BlockChain *> &UpdatedPreds,
+    SmallVectorImpl<MachineBasicBlock *> &BlockWorkList,
+    const BlockFilterSet *BlockFilter = nullptr) {
+  BlockChain &Chain = *BlockToChain[MBB];
+  if (!UpdatedPreds.insert(&Chain).second)
+    return;
+
+  assert(Chain.UnscheduledPredecessors == 0);
+  for (MachineBasicBlock *ChainBB : Chain) {
+    assert(BlockToChain[ChainBB] == &Chain);
+    for (MachineBasicBlock *Pred : ChainBB->predecessors()) {
+      if (BlockFilter && !BlockFilter->count(Pred))
+        continue;
+      if (BlockToChain[Pred] == &Chain)
+        continue;
+      ++Chain.UnscheduledPredecessors;
+    }
+  }
+
+  if (Chain.UnscheduledPredecessors == 0)
+    BlockWorkList.push_back(*Chain.begin());
+}
+
 void MachineBlockPlacement::buildChain(
     MachineBasicBlock *BB, BlockChain &Chain,
     SmallVectorImpl<MachineBasicBlock *> &BlockWorkList,
     const BlockFilterSet *BlockFilter) {
   assert(BB);
   assert(BlockToChain[BB] == &Chain);
   MachineFunction &F = *BB->getParent();
   MachineFunction::iterator PrevUnplacedBlockIt = F.begin();
void MachineBlockPlacement::buildLoopChains(MachineFunction &F,
 
   // FIXME: This is a really lame way of walking the chains in the loop: we
   // walk the blocks, and use a set to prevent visiting a particular chain
   // twice.
   SmallPtrSet<BlockChain *, 4> UpdatedPreds;
   assert(LoopChain.UnscheduledPredecessors == 0);
   UpdatedPreds.insert(&LoopChain);
 
-  for (MachineBasicBlock *LoopBB : LoopBlockSet) {
-    BlockChain &Chain = *BlockToChain[LoopBB];
-    if (!UpdatedPreds.insert(&Chain).second)
-      continue;
-
-    assert(Chain.UnscheduledPredecessors == 0);
-    for (MachineBasicBlock *ChainBB : Chain) {
-      assert(BlockToChain[ChainBB] == &Chain);
-      for (MachineBasicBlock *Pred : ChainBB->predecessors()) {
-        if (BlockToChain[Pred] == &Chain || !LoopBlockSet.count(Pred))
-          continue;
-        ++Chain.UnscheduledPredecessors;
-      }
-    }
-
-    if (Chain.UnscheduledPredecessors == 0)
-      BlockWorkList.push_back(*Chain.begin());
-  }
+  for (MachineBasicBlock *LoopBB : LoopBlockSet)
+    fillWorkLists(LoopBB, UpdatedPreds, BlockWorkList, &LoopBlockSet);
 
   buildChain(LoopTop, LoopChain, BlockWorkList, &LoopBlockSet);
 
   if (RotateLoopWithProfile)
     rotateLoopWithProfile(LoopChain, L, LoopBlockSet);
   else
     rotateLoop(LoopChain, ExitingBB, LoopBlockSet);
 
void MachineBlockPlacement::buildCFGChains(MachineFunction &F) {
 
   // Build any loop-based chains.
   for (MachineLoop *L : *MLI)
     buildLoopChains(F, *L);
 
   SmallVector<MachineBasicBlock *, 16> BlockWorkList;
 
   SmallPtrSet<BlockChain *, 4> UpdatedPreds;
-  for (MachineBasicBlock &MBB : F) {
-    BlockChain &Chain = *BlockToChain[&MBB];
-    if (!UpdatedPreds.insert(&Chain).second)
-      continue;
-
-    assert(Chain.UnscheduledPredecessors == 0);
-    for (MachineBasicBlock *ChainBB : Chain) {
-      assert(BlockToChain[ChainBB] == &Chain);
-      for (MachineBasicBlock *Pred : ChainBB->predecessors()) {
-        if (BlockToChain[Pred] == &Chain)
-          continue;
-        ++Chain.UnscheduledPredecessors;
-      }
-    }
-
-    if (Chain.UnscheduledPredecessors == 0)
-      BlockWorkList.push_back(*Chain.begin());
-  }
+  for (MachineBasicBlock &MBB : F)
+    fillWorkLists(&MBB, UpdatedPreds, BlockWorkList);
 
   BlockChain &FunctionChain = *BlockToChain[&F.front()];
   buildChain(&F.front(), FunctionChain, BlockWorkList);
 
 #ifndef NDEBUG
   typedef SmallPtrSet<MachineBasicBlock *, 16> FunctionBlockSetType;
 #endif
   DEBUG({