[PATCH] D145085: [RISCV] Lower interleaved accesses

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:14878
+  MVT ContainerVT = getContainerForFixedLengthVector(VT.getSimpleVT());
+  unsigned LMUL = RISCVVType::decodeVLMUL(getLMUL(ContainerVT)).first;
+  if (LMUL * Factor > 8)
----------------
Ignoring fractional LMUL here is wrong.  mf8 should count as LMUL1 here I believe.  As you have it written here, you have it treated as LMUL8.

Also, please add a test for this case.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:14879
+  unsigned LMUL = RISCVVType::decodeVLMUL(getLMUL(ContainerVT)).first;
+  if (LMUL * Factor > 8)
+    return false;
----------------
Style: return LMUL * Factor <= 8.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-interleaved-access-zve32x.ll:5
+
+; This checks to make sure that ptr types aren't lowered if XLEN isn't a
+; supported SEW
----------------
We could lower this as a wide load, plus a set of SEW=32 shuffles.  I think the codegen here is distinctly sub-optimal.  I'm not advocating for improving it now, but you might want to adjust the comment to be a bit more future proof.  

p.s. This case is a really good catch.  


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-interleaved-access-zvl32b.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=riscv32 -mattr=+m,+zve32x,+zvl32b -O2 | FileCheck %s -check-prefix=ZVL32B
----------------
The naming on this test file doesn't make any sense to me.

If I'm gathering the intent of these tests correctly, they can be rewritten using wider types using V.  I'd recommend doing so, and merging them into the base fixed-vectors-interleaved-access.ll test.  If you think they really do need the zve32x, maybe into the test file named with that?


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-interleaved-access.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=riscv32 -mattr=+v -O2 | FileCheck %s
+
----------------
Why only riscv32 here?


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-interleaved-access.ll:32
+
+define {<4 x i32>, <4 x i32>, <4 x i32>} @load_factor3(ptr %ptr) {
+; CHECK-LABEL: load_factor3:
----------------
Can you add a few more test variants here?  I realize it's a bit duplicated with the transform test and the intrinsic codegen tests, but having the end to end codegen demonstrated for at least a few of the common factors seems worthwhile.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-shufflevector-vnsrl.ll:7
 
 define void @vnsrl_0_i8(ptr %in, ptr %out) {
 ; CHECK-LABEL: vnsrl_0_i8:
----------------
I'm concerned about the loss of test coverage here.  I think it makes sense to add a runline which explicitly turns off interleave load/store formation.  Or even just do so unconditionally for this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145085