[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.
----------------
I suppose we'll have to promote to i16 for this case?


================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:731
 
-  auto NoSize = [](const SmallSet<TypeSize, 2> &Sizes, MVT T) -> bool {
-    return !Sizes.count(T.getSizeInBits());
+  auto NoSize =
+      [](const SmallSet<std::pair<bool, TypeSize::ScalarTy>, 2> &Sizes,
----------------
craig.topper wrote:
> This hack is to prevent tablegen from throwing a bunch of warnings about assuming a vector type is not scalable. This occurs because of my use of SDTCisSameSizeAs in the type profile for vrgather_vv_vl in tablegen.
> 
> SmallSet defaults to std::less as its comparison function. But TypeSize doesn't implement operator< natively. But it appears to match to the operator< for uint64_t since there's a conversion operator to uint64_t in TypeSize. But that only works for fixed vectors.
> 
> To workaround this, I explicitly split up the TypeSize into a std::pair. Adding an operator< to TypeSize didn't seem like a good idea. Another option might be to switch to SmallDenseSet and add a DenseMapInfo for TypeSize.
Hmm. I agree that `operator<` for `TypeSize` doesn't sound right. I can see the benefit of having a `DenseMapInfo` for `TypeSize`. I guess another option is to overload the comparison for this one case? That's quite short-sighted though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97609