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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 4 inline comments as done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:749
+  case ISD::INSERT_VECTOR_ELT:
+    return lowerINSERTVECTORELT(Op, DAG);
   case ISD::VSCALE: {
----------------
craig.topper wrote:
> Maybe we should keep the underscores in the  name? Or switch to camel case. 
Yeah I agree. I was trying to keep the linter happy but I think it's reasonable to override it here.


================
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);
----------------
craig.topper wrote:
> Is this a copy/paste from lowerSPLATVECTOR?
Hah yeah good catch, thanks.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1323
+  SDValue SplattedIdx =
+      DAG.getSplatVector(VecVT, DL, DAG.getSExtOrTrunc(Idx, DL, MVT::i64));
+
----------------
craig.topper wrote:
> Index isn't signed so this should probably be ZExtOrTrunc?
Makes sense. I had it SExtOrTrunc because it should be correct (?)  and it was easier to identify as a sign-extended 32-bit value in the RV32 SPLAT_VECTOR legalization. With ZExtOrTrunc the codegen is currently worse.

Though I think I need to take another look I think this function might be doing splats wrong, and/or creating illegal values. I'll take a look on Monday.


================
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:
> 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.
Done. I kept it as `XLenVT` rather than `MVT::i32` but it still works to reduce the bulk.


================
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)),
----------------
craig.topper wrote:
> This feels like a pretty complicated output sequence. Should we custom lower it?
Custom-lowered to a `(slideup (vmv (slidedown)))`? Or even a `(slideup (insertelt (slidedown), val, 0))` ?Might be a good idea given the we need slidedown for RV32 legalization anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94615