[PATCH] D144092: [RISCV] Lower interleave and deinterleave intrinsics

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

At a high level, I'm unhappy with the amount of duplication between the scalable path with the new nodes and the fixed vector path.  I think this is a great POC - it largely convinces me we *can* lower these reasonably - but we need to rethink this a bit for landing.

I'm wondering whether we should canonicalize fixed length shuffles for interleave and deinterleave to the new nodes, and then have the lowering as you roughly structured it.  This would avoid some of the code duplication.

The other approach would be to have a set of utility functions, and call them appropriately from both places.

I will note I'm open to being convinced this can be done in tree, but in that case, I'd probably want to see a more minimal lowering with incremental improvement and sharing in follow up changes.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6552
+  if (VecVT.getScalarSizeInBits() < Subtarget.getELEN()) {
+    // Bitcast the concatenated vector from <n x m x ty> -> <n x m / 2 x ty * 2>
+    // This is also casts FPs to ints
----------------
This case looks to be missing from the fixed length lowering, we should find a way to pick it up there as well.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6626
+  if (VecVT.getScalarSizeInBits() < Subtarget.getELEN()) {
+    // We're working with a vector of the same size as the resulting
+    // interleaved vector, but with half the number of elements and
----------------
This bit of code should be shared with the isInterleaveShuffle above.  This will require a bit of restructuring.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.h:273
 
+  // Narrowing logical shift right.
+  // Operands are (source, shift, passthru, mask, vl)
----------------
Unrelated, commit separately please.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144092