[NFC][NARY-REASSOCIATE] Restructure code to aviod isPotentiallyReassociatable

[ebrevnov]

Summary

Currently we have to duplicate the same checks in isPotentiallyReassociatable and tryReassociate. With simple pattern like add/mul this may be not a big deal. But the situation gets much worse when I try to add support for min/max. Min/Max may be represented by several instructions and can take different forms. In order reduce complexity for upcoming min/max support we need to restructure the code a bit to avoid mentioned code duplication.

[mkazantsev]

LGTM

llvm/lib/Transforms/Scalar/NaryReassociate.cpp

291

Return after unreachable not needed.

[ebrevnov]

Rebase

[ebrevnov]

Rebase

llvm/include/llvm/Transforms/Scalar/NaryReassociate.h

bool runImpl(Function &F, AssumptionCache *AC_, DominatorTree *DT_,
                TargetTransformInfo *TTI_);
 
 private:
   // Runs only one iteration of the dominator-based algorithm. See the header
   // comments for why we need multiple iterations.
   bool doOneIteration(Function &F);
 
   // Reassociates I for better CSE.
-  Instruction *tryReassociate(Instruction *I);
+  Instruction *tryReassociate(Instruction *I, const SCEV *&OrigSCEV);
 
   // Reassociate GEP for better CSE.
   Instruction *tryReassociateGEP(GetElementPtrInst *GEP);
 
   // Try splitting GEP at the I-th index and see whether either part can be
   // CSE'ed. This is a helper function for tryReassociateGEP.
   //
   // \p IndexedType The element type indexed by GEP's I-th index. This is

llvm/lib/Transforms/Scalar/NaryReassociate.cpp

bool NaryReassociatePass::runImpl(Function &F, AssumptionCache *AC_,
   bool Changed = false, ChangedInThisIteration;
   do {
     ChangedInThisIteration = doOneIteration(F);
     Changed |= ChangedInThisIteration;
   } while (ChangedInThisIteration);
   return Changed;
 }
 
-// Explicitly list the instruction types NaryReassociate handles for now.
-static bool isPotentiallyNaryReassociable(Instruction *I) {
-  switch (I->getOpcode()) {
-  case Instruction::Add:
-  case Instruction::GetElementPtr:
-  case Instruction::Mul:
-    return true;
-  default:
-    return false;
-  }
-}
-
 bool NaryReassociatePass::doOneIteration(Function &F) {
   bool Changed = false;
   SeenExprs.clear();
   // Process the basic blocks in a depth first traversal of the dominator
   // tree. This order ensures that all bases of a candidate are in Candidates
   // when we process it.
   SmallVector<WeakTrackingVH, 16> DeadInsts;
   for (const auto Node : depth_first(DT)) {
     BasicBlock *BB = Node->getBlock();
     for (auto I = BB->begin(); I != BB->end(); ++I) {
       Instruction *OrigI = &*I;
-
-      if (!SE->isSCEVable(OrigI->getType()) ||
-          !isPotentiallyNaryReassociable(OrigI))
-        continue;
-
-      const SCEV *OrigSCEV = SE->getSCEV(OrigI);
-      if (Instruction *NewI = tryReassociate(OrigI)) {
+      const SCEV *OrigSCEV = nullptr;
+      if (Instruction *NewI = tryReassociate(OrigI, OrigSCEV)) {
         Changed = true;
         OrigI->replaceAllUsesWith(NewI);
 
         // Add 'OrigI' to the list of dead instructions.
         DeadInsts.push_back(WeakTrackingVH(OrigI));
         // Add the rewritten instruction to SeenExprs; the original
         // instruction is deleted.
         const SCEV *NewSCEV = SE->getSCEV(NewI);
for (auto I = BB->begin(); I != BB->end(); ++I) {
         // To alleviate this issue of ScalarEvolution not always capturing
         // equivalence, we add I to SeenExprs[OldSCEV] as well so that we can
         // map both SCEV before and after tryReassociate(I) to I.
         //
         // This improvement is exercised in @reassociate_gep_nsw in
         // nary-gep.ll.
         if (NewSCEV != OrigSCEV)
           SeenExprs[OrigSCEV].push_back(WeakTrackingVH(NewI));
-      } else
+      } else if (OrigSCEV)
         SeenExprs[OrigSCEV].push_back(WeakTrackingVH(OrigI));
     }
   }
   // Delete all dead instructions from 'DeadInsts'.
   // Please note ScalarEvolution is updated along the way.
   RecursivelyDeleteTriviallyDeadInstructionsPermissive(
       DeadInsts, TLI, nullptr, [this](Value *V) { SE->forgetValue(V); });
 
   return Changed;
 }
 
-Instruction *NaryReassociatePass::tryReassociate(Instruction *I) {
+Instruction *NaryReassociatePass::tryReassociate(Instruction * I,
+                                                 const SCEV *&OrigSCEV) {
+
+  if (!SE->isSCEVable(I->getType()))
+    return nullptr;
+
   switch (I->getOpcode()) {
   case Instruction::Add:
   case Instruction::Mul:
+    OrigSCEV = SE->getSCEV(I);
     return tryReassociateBinaryOp(cast<BinaryOperator>(I));
   case Instruction::GetElementPtr:
+    OrigSCEV = SE->getSCEV(I);
     return tryReassociateGEP(cast<GetElementPtrInst>(I));
   default:
-    llvm_unreachable("should be filtered out by isPotentiallyNaryReassociable");
+    return nullptr;
   }
+
+  llvm_unreachable("should not be reached");
+  return nullptr;
 }
 
 static bool isGEPFoldable(GetElementPtrInst *GEP,
                           const TargetTransformInfo *TTI) {
   SmallVector<const Value*, 4> Indices;
   for (auto I = GEP->idx_begin(); I != GEP->idx_end(); ++I)
     Indices.push_back(*I);
   return TTI->getGEPCost(GEP->getSourceElementType(), GEP->getPointerOperand(),