[PATCH] D13697: [SimplifyCFG] Merge conditional stores

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

I think this is fairly close to be ready to check in; mostly minor stuff inline.

While I'm comfortable giving a final LGTM once these comments are addressed, it will be more preferable if you can recruit someone more familiar with SimplifyCFG than me to take a final look.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2353
@@ +2352,3 @@
+  for (auto *BB : {BB1, BB2})
+    if (BB)
+      for (auto &I : *BB)
----------------
I'd use an early `continue` here, will reduce the nesting a bit.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2356
@@ +2355,3 @@
+        if (auto *SI = dyn_cast<StoreInst>(&I))
+          if (SI->getParent() == BB1 || SI->getParent() == BB2) {
+            if (S)
----------------
This check isn't necessary any more -- `SI` will always be either in `BB1` or `BB2`.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2383
@@ +2382,3 @@
+    if (cast<PHINode>(I)->getIncomingValueForBlock(BB) == V)
+      PHI = cast<PHINode>(I);
+
----------------
This will insert a redundant PHI if the first PHI in `Succ` only has one correct incoming value, but is followed by a PHI that has both `V` and `AlternateV` correctly as incoming values.  How about extracting out a lambda that checks if a phi node does what you want (both for `V` and `AlternateV`) and use that in this loop?

This is minor though, and if you don't prefer doing this then I'm fine with it.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2414
@@ +2413,3 @@
+    // Heuristic: if the block can be if-converted/phi-folded and the
+    // instructions inside
+    // are all cheap (arithmetic/GEPs), it's worthwhile to thread this store.
----------------
Fix the wrapping here.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2558
@@ +2557,3 @@
+  // Legality bailouts. We must have at least the non-fallthrough blocks and
+  // the post-donimating block, and the non-fallthroughs must only have one
+  // predecessor.
----------------
Nit: "dominating"


http://reviews.llvm.org/D13697