[PATCH] D13697: [SimplifyCFG] Merge conditional stores

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

I haven't grokked the whole patch yet, but some comments inline based off of what I've understood so far.  Also, please upload the diff with full context.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2385
@@ +2384,3 @@
+      if (PredBB != BB &&
+          PHI->getIncomingValueForBlock(PredBB) != AlternativeV) {
+        PHI = nullptr;
----------------
These are suggestions, I'm not a 100% sure that these will actually make the code more readable:

 - Extract out a `BasicBlock *OtherIncomingBlock` variable

 - Put an assert in the loop verifying that there are only two incoming blocks (or remove the loop and have an if/else in its place).  IOW, make it obvious that we're not dealing with blocks with an arbitrary number of preds.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2416
@@ +2415,3 @@
+  // Now check the stores are compatible.
+  if (QStore->getType() != PStore->getType() ||
+      !QStore->isUnordered() || !PStore->isUnordered())
----------------
I think you need `QStore->getPointerOperand()->getType()` here.  All `StoreInst` s have the type `void`.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2450
@@ +2449,3 @@
+  auto IsDangerous = [](const Instruction &I) {
+    return isa<LoadInst>(I) || isa<StoreInst>(I) || isa<CallInst>(I) ||
+           isa<InvokeInst>(I);
----------------
What about things like `AtomicRMWInst`, `AtomicCmpXchgInst` and `FenceInst`?

I think you're better off querying `Instruction::mayReadOrWriteMemory`.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2517
@@ +2516,3 @@
+  //
+  // This can reduce the number of stores executed if both of the conditions are
+  // true, and can allow the blocks to become small enough to be if-converted.
----------------
However, this is a pessimization if both conditions are always false, and the resulting code does not simplify further, right?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2597
@@ +2596,3 @@
+  
+  SmallVector<Value*,4> CommonAddresses;
+  std::set_intersection(PStoreAddresses.begin(), PStoreAddresses.end(),
----------------
Nit: spacing should be `SmallVector<Value *, 4>`.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2598
@@ +2597,3 @@
+  SmallVector<Value*,4> CommonAddresses;
+  std::set_intersection(PStoreAddresses.begin(), PStoreAddresses.end(),
+                        QStoreAddresses.begin(), QStoreAddresses.end(),
----------------
I'm not sure this is correct -- `std::set_intersection` expects both the ranges to be sorted, and I don't think `SmallPtrSet` is guaranteed to iterate over values in sorted order.  I think it is best to use `llvm::set_intersect` here.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2662
@@ +2661,3 @@
+  // If both branches are conditional and both contain stores to the same
+  // address, remove the stores from the conditionals and create a conditional merged store at the end.
+  if (MergeCondStores && mergeConditionalStores(PBI, BI))
----------------
Wrap the line?

Actually, I'll just assume you'll run clang-format before checkin. :)

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3621
@@ -3342,2 +3620,3 @@
   }
 
+  // If we can prove that the cases must cover all possible values, the
----------------
Unrelated whitespace changes?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5101
@@ -4804,2 +5100,3 @@
   return SimplifyCFGOpt(TTI, BB->getModule()->getDataLayout(),
-                        BonusInstThreshold, AC).run(BB);
+                        BonusInstThreshold, AC)
+      .run(BB);
----------------
Whitespace damage?


Repository:
  rL LLVM

http://reviews.llvm.org/D13697