[PATCH] D13697: [SimplifyCFG] Merge conditional stores

[hfinkel@anl.gov via llvm-commits]

hfinkel added a subscriber: hfinkel.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2349
@@ +2348,3 @@
+// return nullptr.
+static StoreInst *findUniqueStoreInBlocks(Value *Address, BasicBlock *BB1,
+                                          BasicBlock *BB2) {
----------------
This function signature and comment are out of date (Address is not used here).

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2368
@@ +2367,3 @@
+static Value *ensureValueAvailableInSuccessor(Value *V, BasicBlock *BB,
+                                              Value *AlternativeV = nullptr) {
+  // PHI is going to be a PHI node that allows the value V that is defined in
----------------
Please comment on what AlternativeV means (when it is not null).

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2388
@@ +2387,3 @@
+      
+      assert(Succ->getNumUses() == 2);
+      auto PredI = pred_begin(Succ);
----------------
This seems somewhat dangerous as an assertion, unless you specifically check it as a precondition somewhere. Otherwise, it can be violated in non-obvious ways (the block could have its address taken, for example).

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2423
@@ +2422,3 @@
+      if (!isa<BinaryOperator>(I) && !isa<GetElementPtrInst>(I) &&
+          !isa<StoreInst>(I) && !isa<TerminatorInst>(I))
+        return false;
----------------
You also need to skip debug intrinsics. Bitcasts. but only on pointer types, are probably also a good idea.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2499
@@ +2498,3 @@
+  QB.SetInsertPoint(T);
+  QB.CreateStore(QPHI, Address);
+
----------------
You're not preserving here any of the (aliasing) metadata that might have been present on the stores.



Repository:
  rL LLVM

http://reviews.llvm.org/D13697