[PATCH] D51471: [SimplifyCFG] Reduce memory churn during common case for branch commoning [NFC]

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: mkazantsev, anna, jingyue.
Herald added subscribers: llvm-commits, bollu, mcrosier.

I figured this one was worthy of an extra set of eyes despite being reasonable straight forward code wise.  While the patch does exactly what the subject says, the motivation is really to simplify a follow on patch (https://reviews.llvm.org/D51458).  I'm legitimately unsure if it makes sense to land this in isolation, or just role it into the combined patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D51471

Files:
  lib/Transforms/Utils/SimplifyCFG.cpp


Index: lib/Transforms/Utils/SimplifyCFG.cpp
===================================================================
--- lib/Transforms/Utils/SimplifyCFG.cpp
+++ lib/Transforms/Utils/SimplifyCFG.cpp
@@ -2686,40 +2686,51 @@
       PBI->swapSuccessors();
     }
 
-    // If we have bonus instructions, clone them into the predecessor block.
-    // Note that there may be multiple predecessor blocks, so we cannot move
-    // bonus instructions to a predecessor block.
-    ValueToValueMapTy VMap; // maps original values to cloned values
-    // We already make sure Cond is the last instruction before BI. Therefore,
-    // all instructions before Cond other than DbgInfoIntrinsic are bonus
-    // instructions.
-    for (auto BonusInst = BB->begin(); Cond != &*BonusInst; ++BonusInst) {
-      Instruction *NewBonusInst = BonusInst->clone();
-      RemapInstruction(NewBonusInst, VMap,
+    // Ensure all instructions are cloned to the predecessor block, and
+    // identify the copy of Cond in the predecessor.  As a small optimization
+    // for the common case to reduce allocation traffic, we move instructions
+    // if we know there is at most a single predecessor.
+    Instruction *CondInPred = nullptr;
+    if (PredCount == 1) {
+      for (auto BonusInstItr = BB->begin(); Cond != &*BonusInstItr;) {
+        auto BonusInst = BonusInstItr++;
+        BonusInst->moveBefore(PBI);
+        // Remove all potentially control dependent facts (TODO: could be more
+        // fine grained here)
+        BonusInst->dropUnknownNonDebugMetadata();
+      }
+      CondInPred = Cond;
+      CondInPred->moveBefore(PBI);
+    } else {
+      // Handle the general case where we may have more than one predecessor
+      // and thus need to clone rather than move.  
+      ValueToValueMapTy VMap; // maps original values to cloned values
+      // We already make sure Cond is the last instruction before BI. Therefore,
+      // all instructions before Cond other than DbgInfoIntrinsic are bonus
+      // instructions.
+      for (auto BonusInst = BB->begin(); Cond != &*BonusInst; ++BonusInst) {
+        Instruction *NewBonusInst = BonusInst->clone();
+        RemapInstruction(NewBonusInst, VMap,
+                         RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
+        VMap[&*BonusInst] = NewBonusInst;
+        
+        // Remove all potentially control dependent facts (TODO: could be more
+        // fine grained here)
+        BonusInst->dropUnknownNonDebugMetadata();
+        
+        PredBlock->getInstList().insert(PBI->getIterator(), NewBonusInst);
+        NewBonusInst->takeName(&*BonusInst);
+        BonusInst->setName(BonusInst->getName() + ".old");
+      }
+
+      // Clone Cond into the predecessor basic block
+      CondInPred = Cond->clone();
+      RemapInstruction(CondInPred, VMap,
                        RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
-      VMap[&*BonusInst] = NewBonusInst;
-
-      // If we moved a load, we cannot any longer claim any knowledge about
-      // its potential value. The previous information might have been valid
-      // only given the branch precondition.
-      // For an analogous reason, we must also drop all the metadata whose
-      // semantics we don't understand.
-      NewBonusInst->dropUnknownNonDebugMetadata();
-
-      PredBlock->getInstList().insert(PBI->getIterator(), NewBonusInst);
-      NewBonusInst->takeName(&*BonusInst);
-      BonusInst->setName(BonusInst->getName() + ".old");
-    }
-
-    // Clone Cond into the predecessor basic block, and or/and the
-    // two conditions together.
-    Instruction *CondInPred = Cond->clone();
-    RemapInstruction(CondInPred, VMap,
-                     RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
-    PredBlock->getInstList().insert(PBI->getIterator(), CondInPred);
-    CondInPred->takeName(Cond);
-    Cond->setName(CondInPred->getName() + ".old");
-
+      PredBlock->getInstList().insert(PBI->getIterator(), CondInPred);
+      CondInPred->takeName(Cond);
+      Cond->setName(CondInPred->getName() + ".old");
+    }
     if (BI->isConditional()) {
       Instruction *NewCond = cast<Instruction>(
           Builder.CreateBinOp(Opc, PBI->getCondition(), CondInPred, "or.cond"));


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D51471.163236.patch
Type: text/x-patch
Size: 4256 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180830/e7bb4c24/attachment.bin>