[PATCH] D93013: [RISCV] Define vadd/vsub/vrsub intrinsics and lower to V instructions.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:337
+    // RVV intrinsics may have illegal operands.
+    for (auto VT : {MVT::i1, MVT::i8, MVT::i16, MVT::i32})
+      setOperationAction(ISD::INTRINSIC_WO_CHAIN, VT, Custom);
----------------
Is MVT::i1 needed here? The custom lowering only checks i8/i16/i32.

Should we only have i32 as custom with RV64?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2422
+                                const CCValAssign &VA, const SDLoc &DL,
+                                const RISCVTargetLowering *TLI) {
   MachineFunction &MF = DAG.getMachineFunction();
----------------
Pass by reference?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:129
+    // Regardless LMUL = 1, 2, 4 or 8, the register is represented using the
+    // lowerest number of the register group. In V instruction definition, we
+    // only model the instructions using VR for LMUL = 1.
----------------
lowerest->lowest


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:131
+    // only model the instructions using VR for LMUL = 1.
+    SrcReg = RI->getSubReg(SrcReg, RISCV::sub_vrm2);
+    DstReg = RI->getSubReg(DstReg, RISCV::sub_vrm2);
----------------
This will fail for VMV1R_V won't it?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:133
+    DstReg = RI->getSubReg(DstReg, RISCV::sub_vrm2);
+    BuildMI(MBB, MBBI, DL, get(Opc), DstReg)
+        .addReg(SrcReg, getKillRegState(KillSrc));
----------------
I'm not sure I'm comfortable with losing the tracking of the full register write before all machine IR passes run. Can we have a Pseudo instructions for VMV2/4/8_V with the right register classes and let RISCVMCInstLower.cpp expand them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93013