[PATCH] D13697: [SimplifyCFG] Merge conditional stores

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2352
@@ +2351,3 @@
+  StoreInst *S = nullptr;
+  for (User *U : Address->users())
+    if (auto *SI = dyn_cast<StoreInst>(U))
----------------
I'd just iterate through the two blocks instead of iterating over all of `Address`'s uses, unless you have reason to believe that `Address` will have only a small number of uses.  Also see comment on `IsWorthwhile`.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2366
@@ +2365,3 @@
+                                              Value *AlternativeV = nullptr) {
+  // PHI is going to be a PHI node that allows the value V that is defined in
+  // BB to be referenced in BB's only successor.
----------------
Have you considered using `SSAUpdater` here?  This looks like its duplicating logic that already exists there.

Sorry for not bringing this up earlier!

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2417
@@ +2416,3 @@
+  // Now check the stores are compatible.
+  if (QStore->getPointerOperand()->getType() != PStore->getPointerOperand()->getType() ||
+      !QStore->isUnordered() || !PStore->isUnordered())
----------------
Why do you need to check the types?  Aren't they both stores to `Address`?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2424
@@ +2423,3 @@
+  auto IsWorthwhile = [](BasicBlock *BB) {
+    // Heuristic: if the block can be if-converted and the instructions inside
+    // are all cheap (arithmetic/GEPs), it's worthwhile to thread this store.
----------------
When you say "if-converted" what transform are you talking about specifically?  Is it `CodeGen/EarlyIfConversion.cpp` or `CodeGen/IfConversion.cpp` or something else?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2426
@@ +2425,3 @@
+    // are all cheap (arithmetic/GEPs), it's worthwhile to thread this store.
+    if (BB->size() > PHINodeFoldingThreshold)
+      return false;
----------------
If you move this check on the block size to before the calls to `findUniqueStoreInBlocks` then you'll know in `findUniqueStoreInBlocks` that the blocks are guaranteed to be small (assuming `MergeCondStoresAggressively` is `false`) and scanning through all of the instructions in the blocks won't hurt much.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2429
@@ +2428,3 @@
+    for (auto &I : *BB)
+      if (!isa<BinaryOperator>(I) && !isa<GetElementPtrInst>(I) &&
+          !isa<StoreInst>(I) && !isa<TerminatorInst>(I))
----------------
Might be better to have this as a whitelist -- "all instructions that do not touch memory and the stores we already know about" or something like that.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2450
@@ +2449,3 @@
+  // check there are no other memory operations at all.
+  auto IsDangerous = [](const Instruction &I) {
+    return I.mayReadOrWriteMemory();
----------------
I'm not sure what the lambda adds here -- why not directly call `mayReadOrWriteMemory`?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2476
@@ +2475,3 @@
+  // At this point, it's useful to redefine TB to be the common predecessor
+  // if it is currently nullptr.
+  if (!QTB)
----------------
Why do you need to do this?  The only uses of TB I see are the `XStore->getParent() == XTB` checks; and they should be fine with a `nullptr` `XTB`, no?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2487
@@ +2486,3 @@
+
+  IRBuilder<> PB(QFB->getSinglePredecessor()->getFirstInsertionPt());
+  IRBuilder<> QB(PostBB->getFirstInsertionPt());
----------------
This one isn't used.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2544
@@ +2543,3 @@
+
+  bool SwapPCond = false, SwapQCond = false;
+  // Canonicalize fallthroughs to the true branches.
----------------
Minor nit: I'd use `InvertPCond` as "swapping" a predicate means something slightly different in LLVM.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2566
@@ +2565,3 @@
+  auto HasOnePredAndOneSucc = [](BasicBlock *BB, BasicBlock *P, BasicBlock *S) {
+    return BB && BB->getSinglePredecessor() == P &&
+           BB->getSingleSuccessor() == S;
----------------
Why do you need to check `BB` for `nullptr`?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2582
@@ +2581,3 @@
+  SmallPtrSet<Value *,4> PStoreAddresses, QStoreAddresses;
+  if (PTB)
+    for (auto &I : *PTB)
----------------
You can shorten this using initializer lists:

```
for (auto *BB : { PTB, PFB }) {
  if (!BB) continue;
  for (auto &I : *BB)
    if (StoreInst *SI = ...
```

and possibly even more with initializer lists of `std::pair`.


Repository:
  rL LLVM

http://reviews.llvm.org/D13697