[PATCH] D155940: [SimplifyCFG] Transform for redirecting phis between unmergeable BB and SuccBB

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1124-1125
 
-  // Check to see if merging these blocks would cause conflicts for any of the
-  // phi nodes in BB or Succ. If not, we can safely merge.
-  if (!CanPropagatePredecessorsForPHIs(BB, Succ)) return false;
+  // Only when we can not fold bB into Succ, can we only redirect the
+  // predecessors of BB to Succ
+  bool BBPhisMergeable =
----------------
This sentence did not parse to me. Maybe something like this?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1128
+      BBKillable ||
+      CanRedirectPredsOfEmptyBBToSucc(BB, Succ, BBPreds, SuccPreds, CommonPred);
+
----------------
Is it intentional that if BBKillable is true we still call CanRedirectPredsOfEmptyBBToSucc? Should this be `!BBKillable &&`?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1179
     // To avoid processing the same predecessor more than once.
-    SmallPtrSet<BasicBlock *, 8> SeenPreds;
-    // All predecessors of BB will be moved to Succ.
----------------
Why is this set no longer needed? Can no predecessors appear multiple times in this code?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1315
+    //  Everything except CommonPred that jumped to BB now goes to Succ.
+    BB->replaceUsesWithIf(Succ, [BBPreds, CommonPred](Use &use) -> bool {
+      if (Instruction *UseInst = dyn_cast<Instruction>(use.getUser()))
----------------
use -> U


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

https://reviews.llvm.org/D155940