[PATCH] D111574: [SLP]Improve reductions vectorization.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Some minor comments, but its a heavy patch to review tbh...



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3792
+        buildTree_rec(Operands, Depth + 1, {TE, I});
       }
       return;
----------------
Can this 2 iteration for-loop be simplified/split ? 


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4949
   // the initial tree element (may be profitable to shuffle the second gather)
   // or they are extractelements, which form shuffle.
   SmallVector<int> Mask;
----------------
Can you transfer some of this explanation up to AreVectorizableGathers - its not very obvious what is being checked there.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5391
+    return None;
+  }
+
----------------
precommit this?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8365
+          ExtraArgs[TreeN] = V;
+        // Add reduction values. The values are sorted for better vetorization
+        // results.
----------------
vectorization


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8379
+                                             RLI->getPointerOperand())]
+                    .push_back(V);
+                Found = true;
----------------
Split this to stop clang-format heroics?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8387
+                                           LI->getPointerOperand())]
+                  .push_back(V);
+          } else if (auto *EI = dyn_cast<ExtractElementInst>(V)) {
----------------
Split this to stop clang-format heroics?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8392
+                                         EI->getVectorOperand())]
+                .push_back(V);
+          } else if (auto *I = dyn_cast<Instruction>(V)) {
----------------
Split this to stop clang-format heroics?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8517
+          I + 1 < E) {
+        InstructionsState S = getSameOpcode(ReducedVals[I + 1]);
+        if (S.getOpcode() == Instruction::ExtractElement && !S.isAltShuffle()) {
----------------
Can you use another variable name to avoid Wshadow?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8711
+      // Final reduction of not vectorized reduced values.
+      for (unsigned I = 0, E = ReducedVals.size(); I < E; ++I) {
+        ArrayRef<Value *> Candidates = ReducedVals[I];
----------------
for-range loop?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8714
+        for (unsigned Cnt = 0, NumReducedVals = Candidates.size();
+             Cnt < NumReducedVals; ++Cnt) {
+          Value *RdxVal = Candidates[Cnt];
----------------
for-range loop?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111574