[PATCH] D112454: [SLP]Improve/fix reordering of the gathered graph nodes.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2700
+        return None;
+      IsIdentity |= Lane == I;
+      if (CurrentOrder[Lane] != E)
----------------
RKSimon wrote:
> Don't all the lanes need to match for this to be Identity?
Not necessarily, partial matching is good too.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2718
+      }
+      if (*It == E) {
+        *It = I;
----------------
RKSimon wrote:
> Maybe pull E out once and give it a better name, rather than having it redefined by both for loops?
Ok


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2747
         !TE->isAltShuffle()) {
       VFToOrderedEntries[TE->Scalars.size()].insert(TE.get());
+    } else if (TE->State == TreeEntry::NeedToGather) {
----------------
RKSimon wrote:
> It might be slighter easier to read by returning here and breaking the if-else chain - not sure though?
Ok


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2748
       VFToOrderedEntries[TE->Scalars.size()].insert(TE.get());
-    } else if (TE->State == TreeEntry::NeedToGather &&
-               TE->getOpcode() == Instruction::ExtractElement &&
-               !TE->isAltShuffle() &&
-               isa<FixedVectorType>(cast<ExtractElementInst>(TE->getMainOp())
-                                        ->getVectorOperandType()) &&
-               allSameType(TE->Scalars) && allSameBlock(TE->Scalars)) {
-      // Check that gather of extractelements can be represented as
-      // just a shuffle of a single vector.
-      OrdersType CurrentOrder;
-      bool Reuse = canReuseExtract(TE->Scalars, TE->getMainOp(), CurrentOrder);
-      if (Reuse || !CurrentOrder.empty()) {
+    } else if (TE->State == TreeEntry::NeedToGather) {
+      if (TE->getOpcode() == Instruction::ExtractElement &&
----------------
RKSimon wrote:
> are we guaranteed to be TE->State == TreeEntry::NeedToGather at this point? If so assert(TE->State == TreeEntry::NeedToGather) might be better.
We may have scattervectorize here too, at least.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2906
         !TE->isAltShuffle()) {
       OrderedEntries.insert(TE.get());
+    } else if (TE->State == TreeEntry::NeedToGather) {
----------------
RKSimon wrote:
> return and break if-else ?
Ok


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112454/new/

https://reviews.llvm.org/D112454