[SLP] Cleanup: Remove loop around `tryToVectorizeSequence` of PHIs.

[vporpo]

Summary

I think that the loop is not needed.
I added an assertion to check whether a second iteration succeeds in
vectorizing phi noeds, but it never caused a crash.
Anyway, if this is actually used, we don't have a test for it.

[ABataev]

Do not forget that instructions can mutate during vectorization and if at the first iteration they are not compatible, they might be transformed into extractelement instructions on this first iteration, and become compatible on the second one

[vporpo]

So the purpose of this loop is to attempt to vectorize the same PHI seeds that failed previously because the code might have changed in the meantime. But is this something that should be done specifically for PHI nodes? Perhaps we should be doing this for other seed types too ?

[ABataev]

In D148966#4293114, @vporpo wrote:

So the purpose of this loop is to attempt to vectorize the same PHI seeds that failed previously because the code might have changed in the meantime. But is this something that should be done specifically for PHI nodes? Perhaps we should be doing this for other seed types too ?

Generally speaking, yes.

[vporpo]

Since this is not a complete feature and we don't have any tests for it, shouldn't we remove it and add it again if needed ? Wdyt?

[ABataev]

In D148966#4293711, @vporpo wrote:

Since this is not a complete feature and we don't have any tests for it, shouldn't we remove it and add it again if needed ? Wdyt?

Why not complete? Removing this may affect vectorization. If you mean to the test, I can try to add it.

[vporpo]

Why not complete? Removing this may affect vectorization. If you mean to the test, I can try to add it.

It is not complete in the sense that this is just for PHIs and is not supported for other seeds.

It just seems to me that this shouldn't have too much impact, and it is better to have the same logic for all seed types. If we really need this feature we should either enable it for all or completely remove it.

[ABataev]

In D148966#4293758, @vporpo wrote:

Why not complete? Removing this may affect vectorization. If you mean to the test, I can try to add it.

It is not complete in the sense that this is just for PHIs and is not supported for other seeds.

It just seems to me that this shouldn't have too much impact, and it is better to have the same logic for all seed types. If we really need this feature we should either enable it for all or completely remove it.

The fact that it is not ready for other seeds, does not make it non-important for PHIs.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

for (int I = 0, E = Opcodes1.size(); I < E; ++I) {
         continue;
       if (Opcodes1[I]->getValueID() != Opcodes2[I]->getValueID())
         return false;
     }
     return true;
   };
 
   bool HaveVectorizedPhiNodes = false;
-  do {
-    // Collect the incoming values from the PHIs.
-    Incoming.clear();
-    for (Instruction &I : *BB) {
-      PHINode *P = dyn_cast<PHINode>(&I);
-      if (!P)
-        break;
 
-      // No need to analyze deleted, vectorized and non-vectorizable
-      // instructions.
-      if (!VisitedInstrs.count(P) && !R.isDeleted(P) &&
-          isValidElementType(P->getType()))
-        Incoming.push_back(P);
-    }
 
-    // Find the corresponding non-phi nodes for better matching when trying to
-    // build the tree.
-    for (Value *V : Incoming) {
-      SmallVectorImpl<Value *> &Opcodes =
-          PHIToOpcodes.try_emplace(V).first->getSecond();
-      if (!Opcodes.empty())
-        continue;
-      SmallVector<Value *, 4> Nodes(1, V);
-      SmallPtrSet<Value *, 4> Visited;
-      while (!Nodes.empty()) {
-        auto *PHI = cast<PHINode>(Nodes.pop_back_val());
-        if (!Visited.insert(PHI).second)
-          continue;
-        for (Value *V : PHI->incoming_values()) {
-          if (auto *PHI1 = dyn_cast<PHINode>((V))) {
-            Nodes.push_back(PHI1);
-            continue;
-          }
-          Opcodes.emplace_back(V);
-        }
-      }
-    }
 
-    HaveVectorizedPhiNodes = tryToVectorizeSequence<Value>(
-        Incoming, PHICompare, AreCompatiblePHIs,
-        [this, &R](ArrayRef<Value *> Candidates, bool LimitForRegisterSize) {
-          return tryToVectorizeList(Candidates, R, LimitForRegisterSize);
-        },
-        /*LimitForRegisterSize=*/true, R);
-    Changed |= HaveVectorizedPhiNodes;
-    VisitedInstrs.insert(Incoming.begin(), Incoming.end());
-  } while (HaveVectorizedPhiNodes);
 
   VisitedInstrs.clear();
 
   InstSetVector PostProcessInstructions;
   SmallDenseSet<Instruction *, 4> KeyNodes;
   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.