[llvm] r315040 - NewGVN: Factor out duplicate parts of OpIsSafeForPHIOfOps

[Daniel Berlin via llvm-commits]

Author: dannyb
Date: Thu Oct  5 18:33:06 2017
New Revision: 315040

URL: http://llvm.org/viewvc/llvm-project?rev=315040&view=rev
Log:
NewGVN: Factor out duplicate parts of OpIsSafeForPHIOfOps

Modified:
    llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp

Modified: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp?rev=315040&r1=315039&r2=315040&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp Thu Oct  5 18:33:06 2017
@@ -640,9 +640,10 @@ private:
                            SmallPtrSetImpl<Value *> &Visited,
                            MemoryAccess *MemAccess, Instruction *OrigInst,
                            BasicBlock *PredBB);
-
-  bool OpIsSafeForPHIOfOps(Value *Op, Instruction *OrigInst,
-                           const BasicBlock *PHIBlock,
+  bool OpIsSafeForPHIOfOpsHelper(Value *V, const BasicBlock *PHIBlock,
+                                 SmallPtrSetImpl<const Value *> &Visited,
+                                 SmallVectorImpl<Instruction *> &Worklist);
+  bool OpIsSafeForPHIOfOps(Value *Op, const BasicBlock *PHIBlock,
                            SmallPtrSetImpl<const Value *> &);
   void addPhiOfOps(PHINode *Op, BasicBlock *BB, Instruction *ExistingValue);
   void removePhiOfOps(Instruction *I, PHINode *PHITemp);
@@ -2505,22 +2506,19 @@ static bool okayForPHIOfOps(const Instru
          isa<LoadInst>(I);
 }
 
-// Return true if this operand will be safe to use for phi of ops.
-//
-// The reason some operands are unsafe is that we are not trying to recursively
-// translate everything back through phi nodes.  We actually expect some lookups
-// of expressions to fail.  In particular, a lookup where the expression cannot
-// exist in the predecessor.  This is true even if the expression, as shown, can
-// be determined to be constant.
-bool NewGVN::OpIsSafeForPHIOfOps(Value *V, Instruction *OrigInst,
-                                 const BasicBlock *PHIBlock,
-                                 SmallPtrSetImpl<const Value *> &Visited) {
+bool NewGVN::OpIsSafeForPHIOfOpsHelper(
+    Value *V, const BasicBlock *PHIBlock,
+    SmallPtrSetImpl<const Value *> &Visited,
+    SmallVectorImpl<Instruction *> &Worklist) {
+
   if (!isa<Instruction>(V))
     return true;
   auto OISIt = OpSafeForPHIOfOps.find(V);
   if (OISIt != OpSafeForPHIOfOps.end())
     return OISIt->second;
 
+  // Keep walking until we either dominate the phi block, or hit a phi, or run
+  // out of things to check.
   if (DT->properlyDominates(getBlockForValue(V), PHIBlock)) {
     OpSafeForPHIOfOps.insert({V, true});
     return true;
@@ -2531,7 +2529,6 @@ bool NewGVN::OpIsSafeForPHIOfOps(Value *
     return false;
   }
 
-  SmallVector<Instruction *, 4> Worklist;
   auto *OrigI = cast<Instruction>(V);
   for (auto *Op : OrigI->operand_values()) {
     if (!isa<Instruction>(Op))
@@ -2545,40 +2542,29 @@ bool NewGVN::OpIsSafeForPHIOfOps(Value *
       }
       continue;
     }
+    if (!Visited.insert(Op).second)
+      continue;
     Worklist.push_back(cast<Instruction>(Op));
   }
+  return true;
+}
 
+// Return true if this operand will be safe to use for phi of ops.
+//
+// The reason some operands are unsafe is that we are not trying to recursively
+// translate everything back through phi nodes.  We actually expect some lookups
+// of expressions to fail.  In particular, a lookup where the expression cannot
+// exist in the predecessor.  This is true even if the expression, as shown, can
+// be determined to be constant.
+bool NewGVN::OpIsSafeForPHIOfOps(Value *V, const BasicBlock *PHIBlock,
+                                 SmallPtrSetImpl<const Value *> &Visited) {
+  SmallVector<Instruction *, 4> Worklist;
+  if (!OpIsSafeForPHIOfOpsHelper(V, PHIBlock, Visited, Worklist))
+    return false;
   while (!Worklist.empty()) {
     auto *I = Worklist.pop_back_val();
-    // Keep walking until we either dominate the phi block, or hit a phi, or run
-    // out of things to check.
-    //
-    if (DT->properlyDominates(getBlockForValue(I), PHIBlock)) {
-      OpSafeForPHIOfOps.insert({I, true});
-      continue;
-    }
-    // PHI in the same block.
-    if (isa<PHINode>(I) && getBlockForValue(I) == PHIBlock) {
-      OpSafeForPHIOfOps.insert({I, false});
-      OpSafeForPHIOfOps.insert({V, false});
+    if (!OpIsSafeForPHIOfOpsHelper(I, PHIBlock, Visited, Worklist))
       return false;
-    }
-    for (auto *Op : cast<Instruction>(I)->operand_values()) {
-      if (!isa<Instruction>(Op))
-        continue;
-      // See if we already know the answer for this node.
-      auto OISIt = OpSafeForPHIOfOps.find(Op);
-      if (OISIt != OpSafeForPHIOfOps.end()) {
-        if (!OISIt->second) {
-          OpSafeForPHIOfOps.insert({V, false});
-          return false;
-        }
-        continue;
-      }
-      if (!Visited.insert(Op).second)
-        continue;
-      Worklist.push_back(cast<Instruction>(Op));
-    }
   }
   OpSafeForPHIOfOps.insert({V, true});
   return true;
@@ -2697,9 +2683,9 @@ NewGVN::makePossiblePHIOfOps(Instruction
               Op = ValuePHI->getIncomingValue(PredNum);
           }
           // If we phi-translated the op, it must be safe.
-          SafeForPHIOfOps = SafeForPHIOfOps &&
-                            (Op != OrigOp ||
-                             OpIsSafeForPHIOfOps(Op, I, PHIBlock, VisitedOps));
+          SafeForPHIOfOps =
+              SafeForPHIOfOps &&
+              (Op != OrigOp || OpIsSafeForPHIOfOps(Op, PHIBlock, VisitedOps));
         }
         // FIXME: For those things that are not safe we could generate
         // expressions all the way down, and see if this comes out to a