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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6570
+  SmallVector<SDValue, 4> TruncVals;
+  for (unsigned I = 0; I < WideN.getNode()->getNumValues(); I++) {
+    TruncVals.push_back(DAG.getNode(ISD::TRUNCATE, DL,
----------------
WideN.getNode()->getNumValues() should just be NumVals right?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6602
+  MVT XLenVT = Subtarget.getXLenVT();
+  SDValue VL = DAG.getRegister(RISCV::X0, XLenVT);
+  SDValue TrueMask = getAllOnesMask(WideVT, VL, DL, DAG);
----------------
Please use getDefaultVLOps here.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6605
+
+  // If the element type is smaller than ELEN, then we can deinterleave
+  // through vnsrl.wi
----------------
I strongly request we drop the optimized case from the initial patch and come back to these in follow up changes.  I want to focus on having a correct base lowering first.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6635
+
+  MVT IdxVT = WideVT.changeVectorElementTypeToInteger();
+  // Create a vector of even indices {0, 2, 4, ...}
----------------
The index type here doesn't look right.  Say we have two vectors of i8.  On a VLEN>2048 machine, we need the index type to be larger than i8.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6673
+
+  MVT ConcatVT =
+      MVT::getVectorVT(VecVT.getVectorElementType(),
----------------
I think this is the same as WideVT above, but with different naming and computation.  Can you standardize on one or the other please?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6679
+
+  // If the element type is smaller than ELEN, then we can interleave with
+  // vwaddu.vv and vwmaccu.vx
----------------
Please drop the optimized case from the initial patch.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6689
+
+    MVT IdxVT = MVT::getVectorVT(MVT::i16, ConcatVT.getVectorElementCount());
+
----------------
See changeVectorElementType


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6708
+    // Build up the index vector for interleaving the concatenated vector
+    // ...   3   3   2   2   1   1   0   0
+    SDValue Idx = DAG.getNode(ISD::SRL, DL, IdxVT, StepVec,
----------------
I think the comments are backwards here.  I believe the value you're actually computing is:
// 0,0,1,1,2,2,.. etc.

This could simply be a "which order do we write vector lanes in" confusion, but this ordered doesn't match the deinterleave comment above either.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6711
+                              DAG.getSplatVector(IdxVT, DL, One));
+    // ... n+3   3 n+2   2 n+1   1   n   0
+    Idx = DAG.getNode(RISCVISD::ADD_VL, DL, IdxVT, Idx,
----------------
Same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144092