[PATCH] D145485: [IR] Generalize interleave/deinterleave intrinsics to factors > 2

Luke Lau via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 7 10:49:49 PST 2023


luke added inline comments.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:2120-2128
-
-def int_experimental_vector_interleave2   : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
-                                                                  [LLVMHalfElementsVectorType<0>,
-                                                                   LLVMHalfElementsVectorType<0>],
+def int_experimental_vector_interleave   : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
+                                                                  [llvm_vararg_ty],
                                                                   [IntrNoMem]>;
 
-def int_experimental_vector_deinterleave2 : DefaultAttrsIntrinsic<[LLVMHalfElementsVectorType<0>,
----------------
paulwalker-arm wrote:
> luke wrote:
> > This is the main disadvantage of using just one intrinsic to represent all factors: We have to use variadic arguments which complicates the type signature, and we need to do the verification ourselves in Verifier.cpp. 
> > 
> > I also considered just creating separate intrinsics for each interleave factor, but didn't like the  duplication that would be required in `Legalise*Types.cpp`/`SelectionDAGBuilder.cpp`.
> I don't like this change because I feel it makes the intrinsics cumbersome to work with due to them being so open-ended when compare to the current form.
> 
> That said, based on the rational I don't think the change is necessary because when designing the initial support we concluded there was no need for the instrinsics and ISD nodes to follow the same interface and in fact good reasons for them to differ.  This means it's perfectly acceptable to add more intrinsics (i.e. int_experimental_vector_interleave3, int_experimental_vector_interleave4...) that all lower to the same ISD node.  This is why the intrinsic is numbered and the ISD is not.
Thanks for the review. I’m not very strongly opinionated on this, and would be happy to rework this to use separate distinct intrinsics. 

In RISC-V the maximum number of interleaving fields in a load/store is 8, so as long as we have up to interleave8/deinterleave8 that should be fine. Does adding those intrinsics sound like a good way forward?


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vector-deinterleave-fixed.ll:405
+; RV64-NEXT:    ret
+%retval = call {<2 x i64>, <2 x i64>, <2 x i64>} @llvm.experimental.vector.deinterleave.v2i64.v6i64(<6 x i64> %vec)
+ret {<2 x i64>, <2 x i64>, <2 x i64>} %retval
----------------
paulwalker-arm wrote:
> This is not how the intrinsic will be written because the overloaded types is now a struct.  You can see this by passing the test files through opt where you'll see the function name will be `llvm.experimental.vector.deinterleave.sl_v2i64v2i64v2i64s.v6i64`.
Thanks, that explains a lot of what I was seeing. Shouldn’t be a worry any more though if we rework the intrinsic


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145485



More information about the llvm-commits mailing list