[PATCH] D94615: [RISCV][WIP] Add RVV insertelt/extractelt scalable-vector patterns

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:749
+  case ISD::INSERT_VECTOR_ELT:
+    return lowerINSERTVECTORELT(Op, DAG);
   case ISD::VSCALE: {
----------------
Maybe we should keep the underscores in the  name? Or switch to camel case. 


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1316
+  assert(!Subtarget.is64Bit() && VecVT.getVectorElementType() == MVT::i64 &&
+         "Unexpected SPLAT_VECTOR lowering");
+  SDValue Vec = Op.getOperand(0);
----------------
Is this a copy/paste from lowerSPLATVECTOR?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1323
+  SDValue SplattedIdx =
+      DAG.getSplatVector(VecVT, DL, DAG.getSExtOrTrunc(Idx, DL, MVT::i64));
+
----------------
Index isn't signed so this should probably be ZExtOrTrunc?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1655
+    // Extract the lower XLEN bits of the correct vector element.
+    SDValue EltLo = DAG.getNode(RISCVISD::VMV_X_S, DL, Subtarget.getXLenVT(),
+                                Slidedown, Idx);
----------------
frasercrmck wrote:
> @craig.topper is it acceptable to use `VMV_X_S` here, do you think? It relies on the instruction transferring the least-significant XLEN bits even if SEW is larger. However your comment of that node says that the result will never be less than the element size.
I think the code in RISCVTargetLoweringComputeNumSignBitsForTargetNode needs to be updated to return 1 if Op.getOperand(0).getScalarValueSizeInBits() is greater than XLen. Other than that it should be fine.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1655
+    // Extract the lower XLEN bits of the correct vector element.
+    SDValue EltLo = DAG.getNode(RISCVISD::VMV_X_S, DL, Subtarget.getXLenVT(),
+                                Slidedown, Idx);
----------------
craig.topper wrote:
> frasercrmck wrote:
> > @craig.topper is it acceptable to use `VMV_X_S` here, do you think? It relies on the instruction transferring the least-significant XLEN bits even if SEW is larger. However your comment of that node says that the result will never be less than the element size.
> I think the code in RISCVTargetLoweringComputeNumSignBitsForTargetNode needs to be updated to return 1 if Op.getOperand(0).getScalarValueSizeInBits() is greater than XLen. Other than that it should be fine.
Maybe put XLenVT in a variable. I think it might shorten the EltLo and EltHi lines to not need a line wrap? Or maybe just use MVT::i32 since the code already requires that.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td:605
+
+    def : Pat<(vti.Vector (insertelt_node (vti.Vector vti.RegClass:$merge),
+                                          vti.ScalarRegClass:$rs1, uimm5:$rs2)),
----------------
This feels like a pretty complicated output sequence. Should we custom lower it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94615