[PATCH] D94444: [RFC][Scalable] Add scalable shuffle intrinsic to extract evens from a pair of vectors

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for creating this patch!

> I chose to extract the even elements from a pair of vectors (full vector result), rather than a single vector (1/2 width vector result). This is in line with existing fixed shuffle vectors. And can be extended to accept an undef argument if needed. The motivation behind this decision was that we'd want the result vector to be a full vector for performance reasons. It would also map well to SVE's LD2 and UZP1.

Are you also planning to add intrinsics for interleaving?

> How do we feel about the intrinsic name: llvm.experimental.vector.extract.evens(...)?

I quite like the `odd/even` terminology, but would prefer to drop the `s`, as in: "extracting the even elements".

> How do we feel about the ISDNode name: ISD::EXTRACT_EVENS_VECTOR? It could be argued that this set of nodes should have SCALABLE in their names, unless we plan to also allow fixed width arguments as well. Currently the fixed width intrinsics are canonicalized to the existing shuffle vector implementation, so they never reach this ISDNode.

I would favour not to add SCALABLE to that name, because there is nothing limiting these nodes from being used for fixed-width vectors.

> Has anyone thought through all the legalizations that are valid on scalable vectors? Promote and Split are obviously valid. Scalarize is obviously invalid. How about Widen? Widen conflicts with the existing unpacked scalable vectors, so it's not clear if it's possible to do.

You're right, I don't think widening is always possible with the current definition. It's not really about how the vectors are laid out in the registers, taking the even elements of `<vscale x 3 x i32>` means that for each `3 x i32` part you'd need to alternate between selecting the even, odd, even, odd, ... elements. That problem goes away if the intrinsic has the requirement that (at least for scalable vectors) the minimum number of elements needs to be a multiple of 2.



================
Comment at: llvm/docs/LangRef.rst:16201
+of even elements from a pair of input vectors. The result type matches the type
+of the input vectors.
+
----------------
Is it worth adding the clarification that it extracts the even elements from a concatenated vector %vec1:%vec2?


================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:543
 
+  /// EXTRACT_EVENS_VECTOR(VEC1, VEC2) - Returns a vector of all the even
+  /// elements from VEC1 and VEC2. The result vector type will match the input
----------------
Probably the same as above, clarify that it returns a vector containing all even elements of VEC1:VEC2.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6997
+    unsigned NumElts = VT.getVectorElementCount().getKnownMinValue();
+    SmallVector<int, 8> Mask(NumElts, -1);
+    for (unsigned i = 0; i < NumElts; ++i)
----------------
nit: SmallVector chooses a default N since D92522 and https://llvm.org/docs/ProgrammersManual.html recommends leaving out N unless there is a well-motivated choice.


================
Comment at: llvm/lib/IR/Verifier.cpp:5171-5178
+    Assert(ResultTy == Op1Ty,
+           "experimental_vector_extract_evens result must have the same "
+           "type as the input vectors.",
+           &Call);
+    Assert(Op1Ty == Op2Ty,
+           "experimental_vector_extract_evens operands must have the same "
+           "type.",
----------------
Can these two Assert's be merged into one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94444