[SLP][NFC] Cleanup: Add while loop around for loop instead of modifying iterators.

[vporpo]

Summary

Updating the begin/end iterators of the for loop within its body is a bit confusing.
Wrapping it with a while loop makes it explicit that we may iterate more than once.

[ABataev]

I assume the process is restarted not to try to redo the analysis (it looks like useless) but to fix possible issue with the deleted/modified instructions. Could you try to use for (Instruction &I : make_early_inc_range(BB)) instead and avoid outer loop?

[vporpo]

I assume the process is restarted not to try to redo the analysis (it looks like useless) but to fix possible issue with the deleted/modified instructions. Could you try to use for (Instruction &I : make_early_inc_range(BB)) instead and avoid outer loop?

Yeah that may be the issue, but I am getting test failures with it. Could it be that we need to revisit already vectorized instructions?

[ABataev]

In D149712#4316416, @vporpo wrote:

I assume the process is restarted not to try to redo the analysis (it looks like useless) but to fix possible issue with the deleted/modified instructions. Could you try to use for (Instruction &I : make_early_inc_range(BB)) instead and avoid outer loop?

Yeah that may be the issue, but I am getting test failures with it. Could it be that we need to revisit already vectorized instructions?

That's possible, if we emit extractelements. Could you try to do it in reverse order, something like make_early_inc_range(reverse(BB))?

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

do {
     Changed |= HaveVectorizedPhiNodes;
     VisitedInstrs.insert(Incoming.begin(), Incoming.end());
   } while (HaveVectorizedPhiNodes);
 
   VisitedInstrs.clear();
 
   InstSetVector PostProcessInstructions;
   SmallDenseSet<Instruction *, 4> KeyNodes;
+  // This loop helps restart the iteration over the instructions of BB.
+  bool RestartIterationOverBB = true;
+  while (RestartIterationOverBB) {
+    RestartIterationOverBB = false;
+
     for (BasicBlock::iterator it = BB->begin(), e = BB->end(); it != e; ++it) {
       // Skip instructions with scalable type. The num of elements is unknown at
       // compile-time for scalable type.
       if (isa<ScalableVectorType>(it->getType()))
         continue;
 
       // Skip instructions marked for the deletion.
       if (R.isDeleted(&*it))
         continue;
       // We may go through BB multiple times so skip the one we have checked.
       if (!VisitedInstrs.insert(&*it).second) {
         if (it->use_empty() && KeyNodes.contains(&*it) &&
             vectorizeSimpleInstructions(PostProcessInstructions, BB, R,
                                         it->isTerminator())) {
           // We would like to start over since some instructions are deleted
           // and the iterator may become invalid value.
           Changed = true;
-        it = BB->begin();
-        e = BB->end();
+          RestartIterationOverBB = true;
+          break;
         }
         continue;
       }
 
       if (isa<DbgInfoIntrinsic>(it))
         continue;
 
       // Try to vectorize reductions that use PHINodes.
       if (PHINode *P = dyn_cast<PHINode>(it)) {
         // Check that the PHI is a reduction PHI.
         if (P->getNumIncomingValues() == 2) {
           // Try to match and vectorize a horizontal reduction.
           Instruction *Root = getReductionInstr(DT, P, BB, LI);
           if (Root && vectorizeRootInstruction(P, Root, BB, R, TTI)) {
             Changed = true;
-          it = BB->begin();
-          e = BB->end();
-          continue;
+            RestartIterationOverBB = true;
+            break;
           }
         }
         // Try to vectorize the incoming values of the PHI, to catch reductions
         // that feed into PHIs.
         for (unsigned I = 0, E = P->getNumIncomingValues(); I != E; I++) {
           // Skip if the incoming block is the current BB for now. Also, bypass
           // unreachable IR for efficiency and to avoid crashing.
           // TODO: Collect the skipped incoming values and try to vectorize them
           // after processing BB.
           if (BB == P->getIncomingBlock(I) ||
               !DT->isReachableFromEntry(P->getIncomingBlock(I)))
             continue;
 
           // Postponed instructions should not be vectorized here, delay their
           // vectorization.
           if (auto *PI = dyn_cast<Instruction>(P->getIncomingValue(I));
               PI && !PostProcessInstructions.contains(PI))
             Changed |= vectorizeRootInstruction(nullptr, PI,
                                                 P->getIncomingBlock(I), R, TTI);
         }
         continue;
       }
 
-    // Ran into an instruction without users, like terminator, or function call
-    // with ignored return value, store. Ignore unused instructions (basing on
-    // instruction type, except for CallInst and InvokeInst).
+      // Ran into an instruction without users, like terminator, or function
+      // call with ignored return value, store. Ignore unused instructions
+      // (basing on instruction type, except for CallInst and InvokeInst).
       if (it->use_empty() &&
           (it->getType()->isVoidTy() || isa<CallInst, InvokeInst>(it))) {
         KeyNodes.insert(&*it);
         bool OpsChanged = false;
         auto *SI = dyn_cast<StoreInst>(it);
         bool TryToVectorizeRoot = ShouldStartVectorizeHorAtStore || !SI;
         if (SI) {
           auto I = Stores.find(getUnderlyingObject(SI->getPointerOperand()));
           // Try to vectorize chain in store, if this is the only store to the
           // address in the block.
-        // TODO: This is just a temporarily solution to save compile time. Need
-        // to investigate if we can safely turn on slp-vectorize-hor-store
-        // instead to allow lookup for reduction chains in all non-vectorized
-        // stores (need to check side effects and compile time).
+          // TODO: This is just a temporarily solution to save compile time.
+          // Need to investigate if we can safely turn on
+          // slp-vectorize-hor-store instead to allow lookup for reduction
+          // chains in all non-vectorized stores (need to check side effects and
+          // compile time).
           TryToVectorizeRoot = (I == Stores.end() || I->second.size() == 1) &&
                                SI->getValueOperand()->hasOneUse();
         }
         if (TryToVectorizeRoot) {
           for (auto *V : it->operand_values()) {
             // Postponed instructions should not be vectorized here, delay their
             // vectorization.
             if (auto *VI = dyn_cast<Instruction>(V);
                 VI && !PostProcessInstructions.contains(VI))
               // Try to match and vectorize a horizontal reduction.
               OpsChanged |= vectorizeRootInstruction(nullptr, VI, BB, R, TTI);
           }
         }
         // Start vectorization of post-process list of instructions from the
         // top-tree instructions to try to vectorize as many instructions as
         // possible.
-      OpsChanged |= vectorizeSimpleInstructions(PostProcessInstructions, BB, R,
-                                                it->isTerminator());
+        OpsChanged |= vectorizeSimpleInstructions(PostProcessInstructions, BB,
+                                                  R, it->isTerminator());
         if (OpsChanged) {
           // We would like to start over since some instructions are deleted
           // and the iterator may become invalid value.
           Changed = true;
-        it = BB->begin();
-        e = BB->end();
-        continue;
+          RestartIterationOverBB = true;
+          break;
         }
       }
 
       if (isa<CmpInst, InsertElementInst, InsertValueInst>(it))
         PostProcessInstructions.insert(&*it);
     }
+  }
 
   return Changed;
 }
 
 bool SLPVectorizerPass::vectorizeGEPIndices(BasicBlock *BB, BoUpSLP &R) {
   auto Changed = false;
   for (auto &Entry : GEPs) {
     // If the getelementptr list has fewer than two elements, there's nothing