[PATCH] D141924: [IR] Add new intrinsics interleave and deinterleave vectors

[Caroline via Phabricator via llvm-commits]

CarolineConcatto 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>],
+                                                                  [IntrNoMem]>;
+
+def int_experimental_vector_deinterleave2 : DefaultAttrsIntrinsic<[LLVMHalfElementsVectorType<0>,
+                                                                   LLVMHalfElementsVectorType<0>],
----------------
paulwalker-arm wrote:
> These intrinsic definitions don't look to match the langref description or tests.
> 
> The text says:
> ```
> declare <4 x double> @llvm.experimental.vector.interleave2.v2f64(<2 x double> %vec1, <2 x double> %vec2)
> ```
> making `v2f64` the overloaded type from which the others (i.e. the `<4 x double>` return type) are derived.  However, `int_experimental_vector_interleave2` is defined such that the return type is the overloaded type.  You can see this if you run your tests through `opt`. For example, interleave2_nxv2f16 becomes:
> ```
> define <vscale x 4 x half> @interleave2_nxv2f16(<vscale x 2 x half> %vec0, <vscale x 2 x half> %vec1) #0 {
>   %retval = call <vscale x 4 x half> @llvm.experimental.vector.interleave2.nxv4f16(<vscale x 2 x half> %vec0, <vscale x 2 x half> %vec1)
>   ret <vscale x 4 x half> %retval
> }
> ```
> For what it's worth I think the text is correct because overloading the smaller type will look more consistent when adding other shapes. Which is to say I have a slight preference for:
> ```
> <vscale x 4 x half> @llvm.experimental.vector.interleave2.nxv2f16(<vscale x 2 x half>...
> <vscale x 6 x half> @llvm.experimental.vector.interleave3.nxv2f16(<vscale x 2 x half>...
> ```
> That will mean adding `LLVMDoubleElementsVectorType` though, unless there's already a class that'll do what we need.
 I believe we want to keep like this and not create a LLVMDoubleElementsVectorType. AFAIU we should use what we already have available in LLVM. More over if we want more strides in the future we may not even have to use LLVMHalfElementsVectorType.
But again, if you have a good argument about the use of LLVMDoubleElementsVectorType  I can change. ATM I only update the test and the examples to use the biggest size type as overloaded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141924