[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:
> > > > 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).
> Reversing the iteration order in `vectorizeCmpInsts` is not a good solution, because no one expects that the function would walk the array in reverse.
> 
> Using a range seems to be the best option here. This pattern with a `template <typename RangeT>` is being used in several other places throughout LLVM. I am not sure I understand why you don't like it.
It just makes less obvous constraints and requirements for tje parameter, while ArrayRef explicitly provides it. Would be good somehow specify the expected constraints, like iterator_range<T> or something similar.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149993