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

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added a comment.

Paul,
Let me know if I understood correctly.
You are suggesting that:

1. Intrinsic have only 1 vec input and 1 vec output.

  <vscale x 4 x i64> @llvm.experimental.vector.deinterleave.nxv4i64(<vscale x 4 x i64> %vec, i64 2)
  <vscale x 4 x i64> @llvm.experimental.vector.interleave.nxv4i64(<vscale x 4 x i64> %vec, i64 2)



2. ISD node have shapes in and out vectors. For instance, if shape is 2:

  RES0, RES1 = DEINTERLEAVE (VEC0, VEC1)
  RES0, RES1 = INTERLEAVE (VEC0, VEC1)

and all the vector sizes(input and output) are equal

If I understood it correctly, these are my 2 cents:
A)This solution is more configurable than the one we are proposing. So the intrinsic verifier needs to have rules to avoid  mistakes like:

  A.1) Shape not proportional to the minimum number of elements(As you wrote:

> Ty.getKnownMinElementCount() must be devisable by shape

.)

For instance:
It should not be possible to do:

  <vscale x 7 x i64> @llvm.experimental.vector.deinterleave.nxv7i64(<vscale x 7 x i64> %vec, i64 2) 

At the moment the intrinsic is checked by tablegen(TargetSelectionDAG), but as you proposed we need to add code for it in Verifier.cpp. I believe I cannot use tablegen to make sure that shape is proportional to the minimum number of elements

  A.2) Shapes that are not supported. Like:

  <vscale x 12 x i64> @llvm.experimental.vector.interleave.nxv12i64(<vscale x 12 x i64> %vec, i64 4) 

or

  <vscale x 6 x i64> @llvm.experimental.vector.interleave.nxv7i64(<vscale x 6 x i64> %vec, i64 3) 

B) For DEINTERLEAVE we can mix even and odd vectors when extracting without being very clear.
(If I could give my suggestion too, we should not return deinterleave as a concatenated vector, but as a struct of N vectors(2 in this case))
The reason is that:
Imagine we want to deinterleave a vector like **<v6i64><i64 0, i641, i64 2, i64 3, i64 4, i64 5>**, the result stored in **<v6i64>Res** and with a shape equal to **2**. It should also split the vector into even and odds elements.
So the llvm-IR would be something like:

  Res = <v6i64>@llvm.experimental.vector.deinterleave.v6i64(<v6i64><i64 0, i641, i64 2, i64 3, i64 4, i64 5>, i64 2)
  Even= <v2i64> @llvm.extract.vector.v2i64(<v6i64> %Res, i64 0)
  Odd = <v4i64> @llvm.extract.vector.v4i64(<v6i64> %Res, i64 2)

The return vector is:
Res =<v6i64><i64 0, i64 2,  i64 4, i64 1, i64 3, i64 5> 
and  Even and Odd are:
Even= <v2i64><i64 0, i64 2>
Odd =<v4i64><i64 4,i64 1, i64 3, i64 5>
We still have 2 vectors, but wrongly split.

The same would not happen if the deinterleave returns a struct with even and odd vectors. We would need to do something like this to have even and odd:

  Res = {<v3i64>, <v3i64>} @llvm.experimental.vector.deinterleave.v3i64(<v6i64><i64 0, i641, i64 2, i64 3, i64 4, i64 5>, i64 2)
  Even= <3i64> @llvm.extract.element.v3i64({<v3i64>, <3i64> }%Res, i64 0)
  Odd = <3i64> @llvm.extract.element.v3i64({<v3i64>, <v3i64>} %Res, i64 1)

Res = {<v3i64><i64 0, i64 2, i64 4>,<v3i64><i64 1, i64 3, i645>} 
and  Even and Odd are:
Even= <v3i64><i64 0, i64 2, i64 4>
Odd =<v3i64><i64 1, i64 3, i64 5>

I believe that the following is not possible, without having to concatenate after the intrinsic call. Which, IMHO, makes it clear the intention/goal.

  Even = <v2i64> @llvm.extract.vector.v2i64(<v3i64> %Res, i64 0)
  Odd = <v4i64> @llvm.extract.vector.v4i64(<v3i64> %Res, i64 2)

And if we want to be similar to a fixed vector. The mask for deinterleave only returns 1 vector, which could be odd or even and not all concatenated.

If all vectors in the ISDNode are the same size I don't foresee any problem with the legalization. But if they are different, then it becomes complicated, as explained before.

To put it in a nutshell, I would say:
I am fine updating as you suggested, I do not foresee many problems. But  I would like us to agree on one suggestion/solution before doing more significant changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141924