[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: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 &&`?
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.)


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

https://reviews.llvm.org/D155940