[PATCH] D13697: [SimplifyCFG] Merge conditional stores

[Philip Reames via llvm-commits]

reames added a subscriber: reames.

================
Comment at: include/llvm/Transforms/Utils/Local.h:141
@@ -141,1 +140,3 @@
+                 unsigned BonusInstThreshold, AssumptionCache *AC = nullptr,
+                 AliasAnalysis *AA = nullptr);
 
----------------
This seems potentially dangerous.  Since SimplifyCFG doesn't preserve AA, are there any situations where a transform could have invalidated cached information, and then your transform runs?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2411
@@ +2410,3 @@
+
+  for (auto &I : *QFB->getSinglePredecessor())
+    if (!IsNoAlias(&I))
----------------
Is there a simple form of this which doesn't require anything other than trivial AA?  If so, I'd start with that, get the transform working and in, then generalize.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2440
@@ +2439,3 @@
+  // PPHI is going to be a PHI node that allows the value that was going to be
+  // stored in PStore to be referenced from PostBB.
+  //
----------------
There's a good chance that pstore isn't from the conditional block at all.  You might want to check dominance.  Optionally, you can speculate pstore out of the conditional block is it's safe to speculate.  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2465
@@ +2464,3 @@
+
+  // At this point, it's useful to redefine TB if it's nullptr.
+  if (!QTB)
----------------
This long wall of code is hard to follow.  Please use some well named helper functions.  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2509
@@ +2508,3 @@
+  // conditional block contains a store to the same address. Both of these
+  // stores are conditional, so they can't be unconditionally sunk. But it may
+  // be profitable to speculatively sink the stores into one merged store at the
----------------
One think you might consider: are there cases where we can insert a store down the other path?  Doing so in general is clearly a violation of both dereferenceability and the memory model, but what about a conditional store to a dereferenceable location which is known to be thread local?

I ran across a similar case in LICM's store promotion which I'm thinking about implementing since it would really help one of my benchmarks.  Just throwing out the idea in case you find it helpful.  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2563
@@ +2562,3 @@
+
+  if (
+    !PostBB ||
----------------
Spacing.  Clang-format?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2566
@@ +2565,3 @@
+    // PFB must point to QBI and only have one predecessor
+    !PFB || PFB->getSingleSuccessor() != QBI->getParent() ||
+    !PFB->getSinglePredecessor() ||
----------------
This chain of checks is hard to read.  Could it be restructured along with the above code to make it simpler?  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2575
@@ +2574,3 @@
+    (QTB && QTB->getSingleSuccessor() != PostBB) ||
+    // PostBB and QBI must both have only two predecessors
+    PostBB->getNumUses() != 2 || QBI->getParent()->getNumUses() != 2)
----------------
Some of these are legality, some are profitability, some are implementation limits.  Please separate and comment each class.  


Repository:
  rL LLVM

http://reviews.llvm.org/D13697