[PATCH] D107210: [RISCV] Support interleaved load lowering

[Luke via Phabricator via llvm-commits]

luke957 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:971
+  // Ensure the element type is legal.
+  if (ElSize != 8 && ElSize != 16 && ElSize != 32 && ElSize != 64)
+    return false;
----------------
craig.topper wrote:
> luke957 wrote:
> > craig.topper wrote:
> > > Does this need to check if floating point elements are supported?
> > Em, is it enough to check bit size only? I think floating point elements will not produce new bit size.
> You're creating a target specific intrinsic, like Intrinsic::riscv_vlseg2. If the floating point type isn't legal, the backend will error when trying to select the intrinsic. If its not legal and this code doesn't touch it, then the backend will be able to scalarize the loads and shuffles.
Thanks. I understand.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:997
+  if (!isScalableVector) {
+    auto *FVTy = cast<FixedVectorType>(VTy);
+    SVTy =
----------------
craig.topper wrote:
> luke957 wrote:
> > craig.topper wrote:
> > > This is not the correct way to convert a fixed vector type to a scalable vector type. For large vectors this will create a type that maps to something larger than LMUL=8. We need to map fixed vector types to scalable vector types using the logic from RISCVTargetLowering::getContainerForFixedLengthVector which takes into account a user provided VLEN via -riscv-v-vector-bits-min command line option.
> > Em, I'm a little confused here. I understand RISCVTargetLowering::getContainerForFixedLengthVector is used in backend when SDAG is built as this method operates on MVT types. So if we just use opt in the middle-end, is this still needed? Further more, if something larger than LMUL=8 appears, could it be legalized by the backend?
> All conversions from a fixed vector type to a scalable vector must follow the VLEN passed on the command line in -riscv-v-vector-bits-min. The backend should not be splitting a scalable type if the fixed vector type was supposed to be legal type.
Yeah, you are right. It takes me some time to understand that. I think we need to make sure the target specific intrinsic is legal according to  the VLEN passed on the command line in -riscv-v-vector-bits-min, so either fixed length vector or scalable vector need legality check (or conversion) for they might exceed the limit of max(LMUL)==8. It seems to be a little bit complicated. Or maybe we could fix this in another patch?


================
Comment at: llvm/test/Transforms/InterleavedAccess/RISCV/interleaved-accesses.ll:17
+  %v0 = shufflevector <16 x i8> %interleaved.vec, <16 x i8> poison, <8 x i32> <i32 0, i32 2, i32 4, i32 6, i32 8, i32 10, i32 12, i32 14>
+  %v1 = shufflevector <16 x i8> %interleaved.vec, <16 x i8> poison, <8 x i32> <i32 1, i32 3, i32 5, i32 7, i32 9, i32 11, i32 13, i32 15>
+  ret void
----------------
craig.topper wrote:
> luke957 wrote:
> > craig.topper wrote:
> > > Please make sure the tests can't be optimized to nothing. These shuffles aren't used so are allowed to be deleted.
> > Could we guarantee this by carefully using the `RUN` instructions in the beginning of the test case?
> Technically yes, but I wanted to see how this codegened in SelectionDAG and everything got deleted. So I'd rather have a more robust test.
I understand. Address comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107210