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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

First, there was another take on this done in https://reviews.llvm.org/D134438.  That approach tried to introduce interleaving stores and deinterleaving loads, whereas this one separates interleave into it's own set of intrinsics.  I think I like this approach better if there if can be made to work cleanly.

Second, I think we can generalize your intrinsics to handle general interleave and deinterleave without much effort.

For the interleave case, we can simply allow an arbitrary number of vector arguments with matching vector types.  If the input type is <vscale x N x ty> than the result is <vscale x A*N x ty> where A is the (compile time constant) number of arguments.  We could even land the current definition and do this generalization in a follow on if desired.

For the deinterleave case, it's a bit trickier.  I'd like to avoid specific odd/even versions.  One option I see is to add two integer constant arguments to the intrinsic.  The first would be the stride, the second would be the remainder.  So, your "even" variant becomes deinterleave(vec, 2, 0).  One piece that I'm not sure works here is that our result type needs to be a function of the type of the vector argument and the first integer argument.  That may require some custom verification rules.

Given these are in the experimental namespace, I'd could probably be convinced that we should land these and then iterate.



================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:574
 
+  /// VECTOR_DEINTERLEAVE(VEC1, VEC2, IDX) - Returns a deinterleaved subvector
+  /// from VEC1 and VEC2. The vector is deinterleaved with a stride of 2
----------------
The choice here to represent the longer vector type as two vectors which are implicitly concatenated is interesting.  Can you explain why you made that choice?  Is it important for legalization?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:11573
+                           DAG.getConstant(0, DL, MVT::i64));
+  SDValue Hi = DAG.getNode(ISD::EXTRACT_SUBVECTOR, DL, OutVT, InVec,
+                           DAG.getConstant(OutNumElts, DL, MVT::i64));
----------------
Is this actually the semantic of a extract_vector on scalable vectors?  Given e.g. 2, I'd expect to get a vector starting at the 3rd element, not split at the high half.  


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:11580
+  if (!OutVT.isScalableVector()) {
+    SmallVector<int, 8> Mask;
+    for (unsigned i = 0; i < OutNumElts; ++i)
----------------
This should be createStrideMask in VectorUtils.h right?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:11602
+  // Use VECTOR_SHUFFLE for the fixed-length vector to to benefit from
+  // existing combines for recognizing specific deinterleave patterns using
+  // VECTOR_SHUFFLE.
----------------
Comment says deinterleave, fix.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:11605
+  if (!OutVT.isScalableVector()) {
+    SmallVector<int, 8> Mask;
+    unsigned NumElts = InVT.getVectorMinNumElements();
----------------
createInterleaveMask - though, I think you chose a different strategy using the concat_vector here.  Make sure you keep it consistent if you change one part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141924