[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;
+  /// Checks if we still trying to estimate the cost for the same nodes and we
+  /// can delay actual cost estimation (virtual shuffle instruction emission).
----------------
Would this sound more accurate: "While set, we are still trying ..." ?
I just did only see it cleared.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:10396
         // Restore the mask for previous partially matched values.
-        if (Entries.front()->ReorderIndices.empty() &&
-            ((Entries.front()->ReuseShuffleIndices.empty() &&
-              E->Scalars.size() == Entries.front()->Scalars.size()) ||
+        if (Entries.front().front()->ReorderIndices.empty() &&
+            ((Entries.front().front()->ReuseShuffleIndices.empty() &&
----------------
nit: suggest hoisting  "Entries.front().front()"  subexpr and assign to dedicated local variable of type "const TreeEntry *"


================
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)) {
----------------
vdmitrie wrote:
> 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?
> 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?

What about the assumption?  I believe "!InVectors.empty()" is always true. That is why I asked the question about assumption.  This method is called twice in the patch and both have early return when InVectors is empty. 


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:9510
+  assert(NumParts > 0 && NumParts < VL.size() &&
+         "Expectedpoistive number of registers.");
+  Entries.clear();
----------------
RKSimon wrote:
> Expected positive 
Typo: "poistive"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149742