[NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

[kmitropoulou]

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

[kmitropoulou]

ping

[kmitropoulou]

ping

[fhahn]

Could you elaborate on the motivation for removing the helper?

[kmitropoulou]

In D130949#3732343, @fhahn wrote:

Could you elaborate on the motivation for removing the helper?

Nobody else uses it and the code is easier to read.

[kmitropoulou]

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

[asbirlea]

I find it easier to follow what's added to OpSafeForPHIOfOps without the helper, so seems like a reasonable change.

llvm/lib/Transforms/Scalar/NewGVN.cpp

2588

Shouldn't this be continue;?

2597

Same comment (continue;)

[kmitropoulou]

llvm/lib/Transforms/Scalar/NewGVN.cpp

2588

You are right :) Thank you!

2597

Same here :)

[kmitropoulou]

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

[asbirlea]

llvm/lib/Transforms/Scalar/NewGVN.cpp

2919

Could you move this to a separate NFC patch?

[kmitropoulou]

llvm/lib/Transforms/Scalar/NewGVN.cpp

2919

I did not do this change. It is shown here because this change was committed in September 3rd.

[kmitropoulou]

llvm/lib/Transforms/Scalar/NewGVN.cpp

2919

It came from the following commit:

commit fedc59734a44ef7b62c5f389b0cdffd02264b2a9
Author: Kazu Hirata <[e-mail]>
Date: Sat Sep 3 11:17:40 2022 -0700

[llvm] Use range-based for loops (NFC)

[asbirlea]

Can you rebase the patch on ToT then?

[kmitropoulou]

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

[kmitropoulou]

In D130949#3803992, @asbirlea wrote:

Can you rebase the patch on ToT then?

Done :)

[kmitropoulou]

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

[asbirlea]

lgtm.

[kmitropoulou]

Updating D130949: [NFC][NewGVN] Remove OpIsSafeForPHIOfOpsHelper()

llvm/lib/Transforms/Scalar/NewGVN.cpp

private:
 
   void initializeCongruenceClasses(Function &F);
   const Expression *makePossiblePHIOfOps(Instruction *,
                                          SmallPtrSetImpl<Value *> &);
   Value *findLeaderForInst(Instruction *ValueOp,
                            SmallPtrSetImpl<Value *> &Visited,
                            MemoryAccess *MemAccess, Instruction *OrigInst,
                            BasicBlock *PredBB);
-  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);
 
   // Value number an Instruction or MemoryPhi.
   void valueNumberMemoryPhi(MemoryPhi *);
   void valueNumberInstruction(Instruction *);

 
 static bool okayForPHIOfOps(const Instruction *I) {
   if (!EnablePhiOfOps)
     return false;
   return isa<BinaryOperator>(I) || isa<SelectInst>(I) || isa<CmpInst>(I) ||
          isa<LoadInst>(I);
 }
 
-bool NewGVN::OpIsSafeForPHIOfOpsHelper(
-    Value *V, const BasicBlock *PHIBlock,
-    SmallPtrSetImpl<const Value *> &Visited,
-    SmallVectorImpl<Instruction *> &Worklist) {
+// 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<Value *, 4> Worklist;
+  Worklist.push_back(V);
+  while (!Worklist.empty()) {
+    auto *I = Worklist.pop_back_val();
+    if (!isa<Instruction>(I))
+      continue;
 
-  if (!isa<Instruction>(V))
-    return true;
-  auto OISIt = OpSafeForPHIOfOps.find(V);
+    auto OISIt = OpSafeForPHIOfOps.find(I);
     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;
+    if (DT->properlyDominates(getBlockForValue(I), PHIBlock)) {
+      OpSafeForPHIOfOps.insert({I, true});
+      continue;
     }
     // PHI in the same block.
-  if (isa<PHINode>(V) && getBlockForValue(V) == PHIBlock) {
-    OpSafeForPHIOfOps.insert({V, false});
+    if (isa<PHINode>(I) && getBlockForValue(I) == PHIBlock) {
+      OpSafeForPHIOfOps.insert({I, false});
       return false;
     }
 
-  auto *OrigI = cast<Instruction>(V);
+    auto *OrigI = cast<Instruction>(I);
     // When we hit an instruction that reads memory (load, call, etc), we must
     // consider any store that may happen in the loop. For now, we assume the
     // worst: there is a store in the loop that alias with this read.
     // The case where the load is outside the loop is already covered by the
     // dominator check above.
     // TODO: relax this condition
     if (OrigI->mayReadFromMemory())
       return false;
 
+    // Check the operands of the current instruction.
     for (auto *Op : OrigI->operand_values()) {
       if (!isa<Instruction>(Op))
         continue;
       // Stop now if we find an unsafe operand.
       auto OISIt = OpSafeForPHIOfOps.find(OrigI);
       if (OISIt != OpSafeForPHIOfOps.end()) {
         if (!OISIt->second) {
-        OpSafeForPHIOfOps.insert({V, false});
+          OpSafeForPHIOfOps.insert({I, false});
           return false;
         }
         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();
-    if (!OpIsSafeForPHIOfOpsHelper(I, PHIBlock, Visited, Worklist))
-      return false;
   }
   OpSafeForPHIOfOps.insert({V, true});
   return true;
 }
 
 // Try to find a leader for instruction TransInst, which is a phi translated
 // version of something in our original program.  Visited is used to ensure we
 // don't infinite loop during translations of cycles.  OrigInst is the
for (auto *DTN : nodes(DT)) {
     }
   }
 
   // Initialize arguments to be in their own unique congruence classes
   for (auto &FA : F.args())
     createSingletonCongruenceClass(&FA);
 }
 
 void NewGVN::cleanupTables() {
   for (CongruenceClass *&CC : CongruenceClasses) {
     LLVM_DEBUG(dbgs() << "Congruence class " << CC->getID() << " has "
                       << CC->size() << " members\n");
     // Make sure we delete the congruence class (probably worth switching to
     // a unique_ptr at some point.
     delete CC;
     CC = nullptr;
   }