[PATCH] D97609: [RISCV] Add support for VECTOR_REVERSE for scalable vector types.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1496
+    // (0, 1,..., VLMAX-2, VLMAX-1) -> (VLMAX-1, VLMAX-2,..., 1, 0).
+    // FIXME: This implementation doesn't work for vectors of more than 256
+    // elements for SEW==8.
----------------
khchen wrote:
> craig.topper wrote:
> > frasercrmck wrote:
> > > I suppose we'll have to promote to i16 for this case?
> > Yeah, but we don't really have a good way of know if it is needed without just always doing it or deciding based on whether -riscv-vector-bits-max is set and what its value is?
> > 
> > We also can't promote to i16 without splitting for lmul=8.
> Agree, it's really a problem, unfortunately vrgatherei16 can not support i8m8 case directly.
> 
> I think maybe compiler could emit a warning for non-workable case if user specific `-riscv-vector-bits-max`.
Yeah, nothing really works well in this situation. It's just that with `<vscale x 64 x i8>` being legal it's not a problem for the future - it would be silently generating wrong code right now on many architectures.

What if, for now, we decided not to support `i8` unless `-riscv-vector-bits-max` was passed?


================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:718
+namespace {
+struct TypeSizeComparator {
+  bool operator()(const TypeSize &LHS, const TypeSize &RHS) const {
----------------
I think this is an improvement over the std::pair.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97609