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

[Hongyu Chen via Phabricator via llvm-commits]

XChy 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 =
----------------
nikic wrote:
> This sentence did not parse to me. Maybe something like this?
Thanks for polishing my comment.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1128
+      BBKillable ||
+      CanRedirectPredsOfEmptyBBToSucc(BB, Succ, BBPreds, SuccPreds, CommonPred);
+
----------------
nikic wrote:
> Is it intentional that if BBKillable is true we still call CanRedirectPredsOfEmptyBBToSucc? Should this be `!BBKillable &&`?
Both `BBKillable || redirect()` and `!BBKillable && redirect()` do not call CanRedirectPredsOfEmptyBBToSucc when BBKillable is true.
What is different is , that when BBKillable is true, the former BBPhisMergeable is true while the latter is false.
Actually it's logically OK, since when BBKillable is true, whether BBPhisMergeable doesn't have any impact.
I write `BBKillable || redirect()` based on semantic meaning : Phis are mergeable if BB can be killed.


================
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.
----------------
nikic wrote:
> Why is this set no longer needed? Can no predecessors appear multiple times in this code?
BBPreds is a SmallPtrSet, on which we would not iterate the same predecessor multiple times.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1004
+static bool
+CanRedirectPredsOfEmptyBBToSucc(BasicBlock *BB, BasicBlock *Succ,
+                                const SmallPtrSetImpl<BasicBlock *> &BBPreds,
----------------
DianQK wrote:
> Should `clang-tidy` recommend lowercase beginnings?
Yes. But I just follow the original style like `CanPropagatePredecessorsForPHIs`.


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

https://reviews.llvm.org/D155940