[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:33
+static unsigned getRegClassIDForFixedLengthVector(const SelectionDAG *DAG, MVT VT,
+                                            const RISCVSubtarget *Subtarget) {
+  assert(VT.isFixedLengthVector() &&
----------------
Line this up with first argument on the previous line


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:38
+
+  unsigned LMul = Subtarget->getLMULForFixedLengthVector(VT);
+  unsigned RCID;
----------------
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.


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


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:862
+    SDLoc DL(V);
+    unsigned RegClassID = getRegClassIDForFixedLengthVector(CurDAG, V.getSimpleValueType(), Subtarget);
+    auto RC = CurDAG->getTargetConstant(RegClassID, DL, Subtarget->getXLenVT());
----------------
Is this line more than 80 columns? It looks long to me.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:863
+    unsigned RegClassID = getRegClassIDForFixedLengthVector(CurDAG, V.getSimpleValueType(), Subtarget);
+    auto RC = CurDAG->getTargetConstant(RegClassID, DL, Subtarget->getXLenVT());
+    auto NewNode = CurDAG->getMachineNode(TargetOpcode::COPY_TO_REGCLASS, DL, VT, V, RC);
----------------
Just use SDValue. auto doesn't save much here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96352