[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:
> 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.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:995
+  ScalableVectorType *SVTy;
+  bool isScalableVector = isa<ScalableVectorType>(VTy);
+  if (!isScalableVector) {
----------------
craig.topper wrote:
> Please fix this clang-tidy warning
Sorry, I should clang tidy every diff before uploading.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:997
+  if (!isScalableVector) {
+    auto *FVTy = cast<FixedVectorType>(VTy);
+    SVTy =
----------------
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?


================
Comment at: llvm/test/Transforms/InterleavedAccess/RISCV/interleaved-accesses.ll:8
+; CHECK-NEXT:    [[TMP1:%.*]] = bitcast <16 x i8>* [[PTR:%.*]] to i8*
+; CHECK-NEXT:    [[VLSEGN:%.*]] = call { <vscale x 8 x i8>, <vscale x 8 x i8> } @llvm.riscv.vlseg2.nxv8i8.i64(i8* [[TMP1]], i64 1)
+; CHECK-NEXT:    [[TMP2:%.*]] = extractvalue { <vscale x 8 x i8>, <vscale x 8 x i8> } [[VLSEGN]], 1
----------------
craig.topper wrote:
> Why is the VL 1? The vector type has 16 elements, the interleave factor is 2. So the VL should be 8.
Fix this. It's my mistake.


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107210