[PATCH] D133441: [SLP] Vectorize mutual horizontal reductions.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:921
+              ((UserIgnoreList && UserIgnoreList->contains(U)) ||
+              getTreeEntry(U) == &TE))
+            Changed |= VectorizableValues[Scalar].insert(U).second;
----------------
How can the user node be equal to the operand node?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2710-2713
+  DenseMap<Value *, SmallPtrSet<User *, 2>> VectorizableValues;
+
+  bool FoundVectorizableValues;
+
----------------
Not sure this is the best option to keep external vectorizable values, again it does not allow to handle values across different graphs, only across the same instance.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7123-7125
+    if (EU.SkipCost)
+      continue;
+
----------------
This again is too optimistic, you need to try to estimate the cost of extra shuffles, required for the vectorization. 


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7303-7306
+  FoundVectorizableValues =
+      Cost >= -SLPCostThreshold && Cost - ExtractCost < -SLPCostThreshold ?
+      cacheVectorizableValues() : false;
+
----------------
Maybe put it to deleteTree() function and check if the graph-to-be-deleted was built but was not vectorized?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11320
+          if (V.foundVectorizableValues())
+            return ReductionRoot;
           if (!AdjustReducedVals())
----------------
Why return?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133441