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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo 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,
----------------
ABataev wrote:
> 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.
I agree, something like `iterator_range<T>` makes it more explicit. Let me update the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149993