[PATCH] D149993: [SLP][NFC] Cleanup: Separate vectorization of Inserts and CmpInsts.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/SLPVectorizer.h:148
   /// Tries to vectorize \p CmpInts. \Returns true on success.
-  bool vectorizeCmpInsts(ArrayRef<CmpInst *> CmpInsts, BasicBlock *BB,
+  template <typename RangeT>
+  bool vectorizeCmpInsts(RangeT CmpInsts, BasicBlock *BB,
----------------
vporpo wrote:
> ABataev wrote:
> > vporpo wrote:
> > > ABataev wrote:
> > > > Why need a template here since looks like it accepts only ArrayRef?
> > > We are passing an `iterator_range` and there is no `ArrayRef` constructor with an `iterator_range` argument.
> > > I could use the actual type `iterator_range<std::reverse_iterator<SmallSetVector<CmpInst *, 8>::iterator>>` but it is pretty ugly and if we ever need to remove the reverse order at the call-site we would have to change this type too. Wdyt?
> > SmallSetVector class, used to store instruction, has getArrayRef() member function, which can return ArrayRef.
> I don't see how getting the ArrayRef from the SmallSetVector would help. We still need to pass an iterator_range as we need to iterate in reverse.
> The only way this would work is if we reverse the iteration within `vectorizeCmpInsts` which I don't think we should do.
Better to avoid template. Better to pass ArrayRef and reverse it there (adding the explanation, why need to do it).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149993