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

[Shahid via Phabricator via llvm-commits]

ashahid added a comment.

Will commit the tests as NFC.

Seems like I am not getting the mails from phabricator, what shall I do to get the mails?

Checked the patch https://reviews.llvm.org/D43776, seems it will make this patch redundant.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1644
 
+      if (ReuseShuffleIndicies.empty()) {
+        bool ShuffledLoads = true;
----------------
ABataev wrote:
> Why you can do this only if `ReuseShuffleIndicies.empty()`?
This is to avoid the overlapping the UniqueValues reuse logic of your changes.


================
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;
+            }
+          }
----------------
ABataev wrote:
> It is enough just to compare `VL` and `Sorted`. If they are the same, the loads are not shuffled
Sure it is, but this avoids the compare. So I thought having a boolean is preferable.


================
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 "
----------------
ABataev wrote:
> Why you can't do to add vectorized tree entry if `UserTreeIdx == -1`?
My bad, this is not required.


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


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


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2899
     if (TreeEntry *E = getTreeEntry(S.OpValue)) {
+      if (!E->VectorizedValue && !E->JumbleShuffleIndices.empty())
+        return vectorizeTree(E);
----------------
ABataev wrote:
> Why do you need this condition?
In the 2nd test of jumbled-load.ll the two operands of MUL is fed from the same loaded vector. The 1st operand is SHUFFLE of LOAD and the 2nd operand is the gather of the same scalar loads.
Query to getTreeEntry() will always return the node with the same vectorized value and hence both the operand of MUL will be fed the shuffled load.
This check is to avoid this scenario.


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


================
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;
+    }();
----------------
ABataev wrote:
> I rather doubt you need all that stuff. You can use original code
This is required otherwise *multiuse*.ll test as well as PR32086.ll will fail because the lanes were recorded according to the order of scalar loads.


Repository:
  rL LLVM

https://reviews.llvm.org/D36130