[PATCH] D13697: [SimplifyCFG] Merge conditional stores

James Molloy via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 27 05:50:08 PDT 2015


jmolloy marked 8 inline comments as done.
jmolloy added a comment.

Hi Sanjoy,

Thanks again for this fantastic review! I agree with most of the comments ; patch updated.

Cheers,

James


================
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.
----------------
sanjoy wrote:
> Have you considered using `SSAUpdater` here?  This looks like its duplicating logic that already exists there.
> 
> Sorry for not bringing this up earlier!
I've just tried using SSAUpdater. Actually it really doesn't help much - the search for an appropriate already-existing PHI here is more than SSAUpdater can do itself, so replacing this function with SSAUpdater pessimizes some code (we end up with another PHI, which becomes another select).

================
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.
----------------
sanjoy wrote:
> When you say "if-converted" what transform are you talking about specifically?  Is it `CodeGen/EarlyIfConversion.cpp` or `CodeGen/IfConversion.cpp` or something else?
I'm talking about the if-conversion done by SimplifyCFG itself. SimplifyCFG refers to this as PHI node folding (see -phi-node-folding-threshold).

================
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))
----------------
sanjoy wrote:
> 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.
This kind of already is a whitelist, is it not? We're saying that if there's any instruction that isn't one of those types, it's not worthwhile. Those are explicitly the instructions that we believe can be if-converted.

StoreInsts don't matter  because we know (from findUniqueStoreInBlocks) that there can only be zero or one store.

================
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;
----------------
sanjoy wrote:
> Why do you need to check `BB` for `nullptr`?
I don't. Removed.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2582
@@ +2581,3 @@
+  SmallPtrSet<Value *,4> PStoreAddresses, QStoreAddresses;
+  if (PTB)
+    for (auto &I : *PTB)
----------------
sanjoy wrote:
> 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`.
Unfortunately type deduction wasn't clever enough to work out an initializer list of std::pair (and it turns out the std::pair elements end up being const too), so I've just done the simpler transform.


Repository:
  rL LLVM

http://reviews.llvm.org/D13697





More information about the llvm-commits mailing list