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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm 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>,
----------------
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.


================
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
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145485