[PATCH] D125111: [SLP] Make reordering aware of external vectorizable scalar stores.

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4084
+  // it into `ExternalReorderIndices`
+  for (const auto &Pair : PtrToStoresMap) {
+    auto &StoresVec = Pair.second;
----------------
vdmitrie wrote:
> Hi Vasileios.
> 
> May I offer you to make some stylistic changes to this method to improve code readability?
> I was bit lazy to place all comments inline - sorry for that :). It was much easier for me to just apply your patch and use editor locally. So I'm only putting summary here:
> - reduce abuse of "auto"
> - do not create thin lambdas just for minor reducing amount of code.
> - improve Cmp to not call getPointerDiff if we already failed once
> - added type check to Cmp before querying for diff (although I'm not sure whether we can have different store types here)
> - inline Cmp into the call for stable_sort
> - inline stable_sort into CanFormVector
> - outline CanFromVector outside the loop
> - I'm not sure we need to look at each store to find out whether they are all sequential.
>   We probably can take diff between the first and the last ones.
> - use range loop for sequential index traversing
> 
> After applying all these suggestions here is how the method could possibly look:
> 
> SmallVector<BoUpSLP::OrdersType, 1>
> BoUpSLP::findExternalStoreUsersReorderIndices(TreeEntry *TE) const {
>   unsigned NumLanes = TE->Scalars.size();
> 
>   DenseMap<Value *, SmallVector<StoreInst *, 4>> PtrToStoresMap =
>       collectUserStores(TE);
> 
>   // Holds the reorder indices for each candidate store vector that is a user of
>   // the current TreeEntry.
>   SmallVector<OrdersType, 1> ExternalReorderIndices;
> 
>   // \Returns true if the stores in `SortedStoresVec` are consecutive and can
>   // form a vector.
>   auto &&CanFormVector = [this](SmallVector<StoreInst *, 4> &StoresVecSorted) {
>     Type *Ty = StoresVecSorted.front()->getValueOperand()->getType();
>     bool FailedToSort = false;
>     stable_sort(StoresVecSorted, [this, &FailedToSort, Ty](StoreInst *S1,
>                                                            StoreInst *S2) {
>       if (FailedToSort)
>         return false;
>       if (S1->getValueOperand()->getType() != Ty ||
>           S2->getValueOperand()->getType() != Ty) {
>         FailedToSort = true;
>         return false;
>       }
>       Optional<int> Diff = getPointersDiff(Ty, S2->getPointerOperand(), Ty,
>                                            S1->getPointerOperand(), *DL, *SE,
>                                            /*StrictCheck=*/true);
>       if (!Diff) {
>         FailedToSort = true;
>         return false;
>       }
>       return Diff < 0;
>     });
>     // If we failed to compare stores, then just abandon this stores vector.
>     if (FailedToSort)
>       return false;
>     Optional<int> Range =
>         getPointersDiff(Ty, StoresVecSorted.front()->getPointerOperand(), Ty,
>                         StoresVecSorted.back()->getPointerOperand(), *DL, *SE,
>                         /*StrictCheck=*/true);
>     return *Range == (int)StoresVecSorted.size() - 1;
>   };
> 
>   // Now inspect the stores collected per pointer and look for vectorization
>   // candidates. For each candidate calculate the reorder index vector and push
>   // it into `ExternalReorderIndices`
>   for (const auto &Pair : PtrToStoresMap) {
>     const SmallVector<StoreInst *, 4> &StoresVec = Pair.second;
> 
>     // If we have fewer than NumLanes stores, then we can't form a vector.
>     if (StoresVec.size() != NumLanes)
>       continue;
> 
>     // Sort the vector based on the pointers. We create a copy because we may
>     // need the original later for calculating the reorder (shuffle) indices.
>     auto StoresVecSorted = StoresVec;
> 
>     // If the stores are not consecutive then abandon this sotres vector.
>     if (!CanFormVector(StoresVecSorted))
>       continue;
> 
>     // The scalars in StoresVec can form a vector instruction, so calculate the
>     // shuffle indices.
>     ExternalReorderIndices.resize(ExternalReorderIndices.size() + 1);
>     OrdersType &ReorderIndices = ExternalReorderIndices.back();
>     for (StoreInst *SI : StoresVec) {
>       unsigned Idx = llvm::find(StoresVecSorted, SI) - StoresVecSorted.begin();
>       ReorderIndices.push_back(Idx);
>     }
> 
>     // Identity order (e.g., {0,1,2,3}) is modeled as an empty OrdersType in
>     // reorderTopToBottom() and reorderBottomToTop(), so we are following the
>     // same convention here.
>     auto IsIdentityOrder = [](const OrdersType &Order) {
>       for (unsigned Idx : seq<unsigned>(0, Order.size()))
>         if (Idx != Order[Idx])
>           return false;
>       return true;
>     };
>     if (IsIdentityOrder(ReorderIndices))
>       ReorderIndices.clear();
>   }
>   return ExternalReorderIndices;
> }
>  
I think I addressed most of these points. Please take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125111