[PATCH] D36130: [SLP] Vectorize jumbled memory loads.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

By the way, take a look at my https://reviews.llvm.org/D43776 that does the same but in more general way



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1644
 
+      if (ReuseShuffleIndicies.empty()) {
+        bool ShuffledLoads = true;
----------------
Why you can do this only if `ReuseShuffleIndicies.empty()`?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1649-1654
+          for (unsigned I = 0, E = Sorted.size() - 1; I < E; ++I) {
+            if (!isConsecutiveAccess(Sorted[I], Sorted[I + 1], *DL, *SE)) {
+              ShuffledLoads = false;
+              break;
+            }
+          }
----------------
It is enough just to compare `VL` and `Sorted`. If they are the same, the loads are not shuffled


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1657
+          // would be useful.
+          if (ShuffledLoads && UserTreeIdx != -1) {
+            DEBUG(dbgs() << "SLP: added a vector of loads which needs "
----------------
Why you can't do to add vectorized tree entry if `UserTreeIdx == -1`?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1660
+                            "permutation of loaded lanes.\n");
+            newTreeEntry(Sorted, true, UserTreeIdx, ReuseShuffleIndicies, Mask);
+            return;
----------------
Each `true` or `false` argument must have to prepend comment with the name of the function parameter, related to this argument


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2279
+        VecLdCost += TTI->getShuffleCost(
+            TargetTransformInfo::SK_PermuteSingleSrc, VecTy, 0);
+      }
----------------
You can remove the last argument here


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2899
     if (TreeEntry *E = getTreeEntry(S.OpValue)) {
+      if (!E->VectorizedValue && !E->JumbleShuffleIndices.empty())
+        return vectorizeTree(E);
----------------
Why do you need this condition?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3251
 
-      LoadInst *LI = cast<LoadInst>(VL0);
+      LoadInst *LI = cast<LoadInst>(VL0);;
       Type *ScalarLoadTy = LI->getType();
----------------
Restore the original code here


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3287
       ++NumVectorInstructions;
+
       return V;
----------------
Remove this empty line


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3528-3533
+    Value *Vec = [&]() {
+      if (auto *SVI = dyn_cast<ShuffleVectorInst>(E->VectorizedValue))
+        if (!E->JumbleShuffleIndices.empty() && isa<LoadInst>(SVI->getOperand(0)))
+          return SVI->getOperand(0);
+      return E->VectorizedValue;
+    }();
----------------
I rather doubt you need all that stuff. You can use original code


================
Comment at: test/Transforms/SLPVectorizer/X86/external_user_jumbled_load.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -S -mtriple=x86_64-unknown -mattr=+avx -slp-vectorizer | FileCheck %s
----------------
You need to add this test separately and show changes in it.


================
Comment at: test/Transforms/SLPVectorizer/X86/jumbled-load-shuffle-placement.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -S -mtriple=x86_64-unknown -mattr=+avx -slp-vectorizer | FileCheck %s
----------------
You need to add this test separately and show changes in it


================
Comment at: test/Transforms/SLPVectorizer/X86/jumbled-load-used-in-phi.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -S -mtriple=x86_64-unknown -mattr=+avx -slp-vectorizer | FileCheck %s
----------------
You need to add this test separately and show changes in it


================
Comment at: test/Transforms/SLPVectorizer/X86/jumbled-load.ll:64
+
+define i32 @jumbled-load-multiuses(i32* noalias nocapture %in, i32* noalias nocapture %out) {
+; CHECK-LABEL: @jumbled-load-multiuses(
----------------
You need to add this test separately and show changes in it


Repository:
  rL LLVM

https://reviews.llvm.org/D36130