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

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4084
+  // it into `ExternalReorderIndices`
+  for (const auto &Pair : PtrToStoresMap) {
+    auto &StoresVec = Pair.second;
----------------
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;
}
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125111