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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7146
+  }
+  void estimateNodesPermuteCost(const TreeEntry &E1, const TreeEntry *E2,
+                                ArrayRef<int> Mask, unsigned Part,
----------------
RKSimon wrote:
> Add method description, also why is it called estimateNodesPermuteCost when it doesn't seem to use/return costs?
It adds the cost to final cost estimation, see e.g. lines 7169 and 7176.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7155
+          (!E2 && InVectors.size() == 1 &&
+           InVectors.front().get<const TreeEntry *>() == &E1)) {
+        ArrayRef<int> SubMask =
----------------
RKSimon wrote:
> This logic makes very little sense to me
If we decided to permute E1 and (possibly) E2 already, and we found that we again need to permute the same nodes, no need to do it immediately, instead need to include the sub-Mask into CommonMask and do it later to avoid double shuffling of the same nodes.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7647
+        else
+          Estimator.add(*TEs.front(), *TEs.back(), VecMask);
+      }
----------------
RKSimon wrote:
> Would we be better off merging the 1TE + 2TE add() methods?
I'll try to rework this function to avoid it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149742