[PATCH] D149742: [SLP]Improve isGatherShuffledEntry by trying per-register shuffle.

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6991
   constexpr static TTI::TargetCostKind CostKind = TTI::TCK_RecipThroughput;
+  bool SameNodesEstimated = true;
 
----------------
I'm not sure I understand the purpose of this flag. Could you please describe?
It is quite confusing that it is already set right at construction when no estimation was done yet.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7255
+           InVectors.back().get<const TreeEntry *>() == E2) ||
+          (!E2 && InVectors.size() >= 1 &&
+           InVectors.front().get<const TreeEntry *>() == &E1)) {
----------------
InVectors.size() >=1  is same as  !InVectors.empty()

Having this check assumes that InVectors can be empty here, but at the same time InVectors.front() below (at line 7267) assumes it is not empty. So which one  is the right assumption?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7718
     std::optional<TargetTransformInfo::ShuffleKind> ExtractShuffle;
-    std::optional<TargetTransformInfo::ShuffleKind> GatherShuffle;
-    SmallVector<const TreeEntry *> Entries;
+    SmallVector<std::optional<TargetTransformInfo::ShuffleKind>> GatherShuffle;
+    SmallVector<SmallVector<const TreeEntry *>> Entries;
----------------
perhaps worth renaming to GatherShuffles since it becomes an array of optionals


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7720
+    SmallVector<SmallVector<const TreeEntry *>> Entries;
+    Type *ScalarTy = GatheredScalars.front()->getType();
     // Check for gathered extracts.
----------------
This ScalarTy declaration hides one at the function scope. 


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7729
+    unsigned NumParts = TTI->getNumberOfParts(FixedVectorType::get(
+        GatheredScalars.front()->getType(), GatheredScalars.size()));
+    if (NumParts == 0 || NumParts >= GatheredScalars.size())
----------------
 ScalarTy ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7740
         }
+    } else if (auto *VecTy =
+                   FixedVectorType::get(VL.front()->getType(), VL.size());
----------------
VecTy is already defined at 7674. Can just reuse it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149742