[PATCH] D116688: [SLP]Excluded external uses from the reprdering estimation.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1143-1144
+    /// \returns The additional cost due to possible broadcasting of the
+    /// elements in the lane. It is more profitable to have power-of-2 unique
+    /// elements in the lane, it will be vectorized with higher probability.
+    int getSplatCost(unsigned Lane, unsigned OpIdx, unsigned Idx) const {
----------------
vporpo wrote:
> Please explain in the comments why it is more profitable.
Do you suggest to ad thу explanation here?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1146
+    int getSplatCost(unsigned Lane, unsigned OpIdx, unsigned Idx) const {
+      Value *V = getData(Idx, Lane).V;
+      if (!isa<Instruction>(V) || V == getData(OpIdx, Lane).V)
----------------
vporpo wrote:
> This needs a better name because it gets a bit confusing later on when you introduce `OpV`.  Perhaps call this `OpIdxV` and the other one `IdxV` ?
Ok


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1150
+      SmallPtrSet<Value *, 4> Uniques;
+      for (unsigned I = 0, E = getNumLanes(); I < E; ++I) {
+        if (I == Lane)
----------------
vporpo wrote:
> Could you rename `I` to `Ln` ?
Ok


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1159-1161
+      int UniquesCountWithV = UniquesCount;
+      if (!Uniques.contains(V))
+        ++UniquesCountWithV;
----------------
vporpo wrote:
> Perhaps replace these lines with:
> `int UniquesCountWithV = Uniques.contains(V) ? UniquesCount  : UniquesCount + 1;`
Ok


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1270
+    /// best operand (with the highest score) between the passes.
+    SmallDenseMap<std::pair<unsigned, unsigned>, unsigned, 8>
+        BestScoresPerLanes;
----------------
vporpo wrote:
> Could you add to the comment about the what each `unsigned` is in the pair? I think it is operand index and lane.
Sure, will do


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1327
+          int Score = getLookAheadScore(OpLeft, OpRight, MainAltOps);
+          if (Score) {
+            int SplatScore = getSplatCost(Lane, OpIdx, Idx);
----------------
vporpo wrote:
> Why skip if score is 0 ?
Score 0 means failed match, it won't be vectorized for sure so no need to check for the reused scalars.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1328
+          if (Score) {
+            int SplatScore = getSplatCost(Lane, OpIdx, Idx);
+            if (Score <= SplatScore)
----------------
vporpo wrote:
> Is it Score or Cost? Score usually suggests the higher the better (and Cost the lower the better).
It is cost currently, will rework it to score.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3047
+      // Do not include ordering for nodes used in the alt opcode vectorization,
+      // better to reorder them during bottom-to-top stage.
+      SmallVector<const TreeEntry *, 1> Worklist(1, TE.get());
----------------
vporpo wrote:
> Why is it better to do this bottom-up ?
> Could this be a separate patch?
Yes, see D116740


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116688