[llvm] [SLP] More OOP to simplify vectorizeStores() (NFC) (PR #134605)

[Sander de Smalen via llvm-commits]

=?utf-8?q?Gaëtan?= Bossu <gaetan.bossu at arm.com>,
=?utf-8?q?Gaëtan?= Bossu <gaetan.bossu at arm.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/134605 at github.com>


================
@@ -20851,52 +20887,35 @@ bool SLPVectorizerPass::vectorizeStores(
   // dependencies and no need to waste compile time to try to vectorize them.
   // - Try to vectorize the sequence {1, {1, 0}, {3, 2}}.
   auto FillStoresSet = [&](unsigned Idx, StoreInst *SI) {
-    for (RelatedStoreInsts &StoreSeq : SortedStores) {
-      std::optional<int> Diff = getPointersDiff(
-          Stores[StoreSeq.BaseInstrIdx]->getValueOperand()->getType(),
-          Stores[StoreSeq.BaseInstrIdx]->getPointerOperand(),
-          SI->getValueOperand()->getType(), SI->getPointerOperand(), *DL, *SE,
-          /*StrictCheck=*/true);
-      if (!Diff)
-        continue;
-      std::optional<unsigned> PrevInst =
-          StoreSeq.insertOrLookup(/*InstrIdx=*/Idx, /*PtrDist=*/*Diff);
-      if (!PrevInst) {
-        // No store was associated to that distance. Keep collecting.
-        return;
-      }
-      // Try to vectorize the first found set to avoid duplicate analysis.
-      TryToVectorize(StoreSeq.Instrs);
-      RelatedStoreInsts::DistToInstMap PrevSet;
-      copy_if(StoreSeq.Instrs, std::inserter(PrevSet, PrevSet.end()),
-              [&](const std::pair<int, unsigned> &DistAndIdx) {
-                return DistAndIdx.second > *PrevInst;
-              });
-      StoreSeq.reset(Idx);
-      // Insert stores that followed previous match to try to vectorize them
-      // with this store.
-      unsigned StartIdx = *PrevInst + 1;
-      SmallBitVector UsedStores(Idx - StartIdx);
-      // Distances to previously found dup store (or this store, since they
-      // store to the same addresses).
-      SmallVector<int> Dists(Idx - StartIdx, 0);
-      for (auto [PtrDist, InstIdx] : reverse(PrevSet)) {
-        // Do not try to vectorize sequences, we already tried.
-        if (VectorizedStores.contains(Stores[InstIdx]))
-          break;
-        unsigned BI = InstIdx - StartIdx;
-        UsedStores.set(BI);
-        Dists[BI] = PtrDist - *Diff;
-      }
-      for (unsigned I = StartIdx; I < Idx; ++I) {
-        unsigned BI = I - StartIdx;
-        if (UsedStores.test(BI))
-          StoreSeq.insertOrLookup(I, Dists[BI]);
-      }
+    std::optional<int> Diff;
+    auto *RelatedStores =
+        find_if(SortedStores, [&](const RelatedStoreInsts &StoreSeq) {
+          StoreInst &BaseStore = StoreSeq.getBaseStore();
+          Diff = getPointersDiff(BaseStore.getValueOperand()->getType(),
+                                 BaseStore.getPointerOperand(),
+                                 SI->getValueOperand()->getType(),
+                                 SI->getPointerOperand(), *DL, *SE,
+                                 /*StrictCheck=*/true);
+          return Diff.has_value();
----------------
sdesmalen-arm wrote:

I don't see the issue with using `find_if` as long as the lambda is trivial, which it is. To avoid concerns with side-effects, maybe you could capture some of the values explicitly?

Similar to your suggestion above, I think adding a (simpler) pointer-diff method in `RelatedStoreInsts` would be useful, which tells whether a given Store is related (and if so, returns an offset). I was thinking something like `bool isRelatedStore(const StoreInst *SI, int &PtrDiff)` (+ args for SE/DL), but if you prefer returning it directly through `std::optional` I'm happy with that too.

https://github.com/llvm/llvm-project/pull/134605