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

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.

LG



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1128
+      BBKillable ||
+      CanRedirectPredsOfEmptyBBToSucc(BB, Succ, BBPreds, SuccPreds, CommonPred);
+
----------------
XChy wrote:
> nikic wrote:
> > XChy wrote:
> > > 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.
> > In particular, what worries me here is that we set BBKillable = true, then call CanRedirectPredsOfEmptyBBToSucc() which may set CommonPred (even if the function ultimately returns false) and then various code in the following will do things differently based on CommonPred. It's not clear to me whether this will result in misbehavior or not.
> > 
> > (With that in mind, I think it may also be cleaner if the BBPhisMergeable flag is removed and only CommonPred left, so that CommonPred != nullptr is equivalent to BBPhisMergeable and we don't leave dangling CommonPred if CanRedirectPredsOfEmptyBBToSucc exits early.)
> What you're worried about make sense to me. But as far as I know, the assumption "`BBKillable || redirect()` will call redirect() when BBKillable is true" is wrong。
Yeah, sorry, I got confused here.


================
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.
----------------
XChy wrote:
> nikic wrote:
> > XChy wrote:
> > > 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.
> > Oh, I missed that this does the updates based on SmallPtrSet now. This is not allowed, because it makes the updates non-deterministic (order matters here). You should replace SmallPtrSet with SmallSetVector to keep order deterministic.
> Sure, restore to original codem though I don't know why order matters here. Because the algorithm for updating dom-tree?
The structure of the dominator tree will be the same regardless of update order, but the order of the children in the tree may be different, which can lead to optimization differences.


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

https://reviews.llvm.org/D155940