[PATCH] D96352: [RISCV] Initial support for insert/extract subvector

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:850
+    // Bail when not a "cast" like insert_subvector.
+    if (cast<ConstantSDNode>(Node->getOperand(2)) != 0)
+      break;
----------------
I'm not sure what this is checking now. cast should return a pointer, and this is checking that the pointer isn't null I think? Which it shouldn't be null so now I"m not sure how this code passes.

My suggestion was to use "Node->getConstantOperandVal(2) != 0"


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:866
+        CurDAG->getTargetConstant(RegClassID, DL, Subtarget->getXLenVT());
+    auto NewNode =
+        CurDAG->getMachineNode(TargetOpcode::COPY_TO_REGCLASS, DL, VT, V, RC);
----------------
This should probably also not use auto. At the very least it needs to have the * so its obvious it is a pointer as clang-tidy is suggesting.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:873
+    // Bail when not a "cast" like extract_subvector.
+    if (cast<ConstantSDNode>(Node->getOperand(1))->getZExtValue() != 0)
+      break;
----------------
Node->getConstantOperandVal(2) != 0


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:38
+
+  unsigned LMul = Subtarget->getLMULForFixedLengthVector(VT);
+  unsigned RCID;
----------------
arcbbb wrote:
> craig.topper wrote:
> > Would it be simpler to get the LMUL from the size of the fixed part of the scalable type? We don't need subtarget for that.
> I was considering it was possible to insert/extract an M1 fixed-width vector into/from an M2 scalable vector, so I get the LMUL from the fixed-width one.
If that happened. I think we would need to create an EXTRACT_SUBREG/INSERT_SUBREG for the register allocator to make sense of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96352