[PATCH] D151396: [2/N][RISCV] Model vxrm in LLVM intrinsics and machine instructions for RVV fixed-point instructions

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:118
+  HasRoundModeOpMask = 1 << HasRoundModeOpShift,
+
 };
----------------
Extra blank line


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertReadWriteCSR.cpp:1
+//===-- RISCVTargetMachine.cpp - Define TargetMachine for RISC-V ----------===//
+//
----------------
Incorrect file name and description


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertReadWriteCSR.cpp:11
+//
+// Currently the pass implements naiive insertion a write to vxrm before an RVV
+// fixed-point instruction.
----------------
naive*

"insertion a write" -> "insertion of a write"?


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertReadWriteCSR.cpp:83
+
+      unsigned VXRMImm = MI.getOperand(RoundModeIdx.value()).getImm();
+      BuildMI(MBB, MI, MI.getDebugLoc(), TII->get(RISCV::WriteVXRMImm))
----------------
Use `operator*` instead of `optional::value`. See many patches like 1792821c8308755593c114c3b7ae8ce33bdb08e9


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:2037
+  let VLMul = MInfo.value in {
+    def "_" # MInfo.MX : VPseudoBinaryNoMaskRoundingMode<RetClass, Op1Class, Op2Class,
+                                             Constraint>;
----------------
80 characters


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:2038
+    def "_" # MInfo.MX : VPseudoBinaryNoMaskRoundingMode<RetClass, Op1Class, Op2Class,
+                                             Constraint>;
+    def "_" # MInfo.MX # "_TU" : VPseudoBinaryNoMaskTURoundingMode<RetClass, Op1Class, Op2Class,
----------------
This isn't indented far enought to line up with RetClass.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:6713
 defm : VPatBinaryV_VV_VX<"int_riscv_vasub", "PseudoVASUB", AllIntegerVectors>;
+defm : VPatBinaryV_VV_VXRoundingMode<"int_riscv_vaadd", "PseudoVAADD", AllIntegerVectors>;
 
----------------
I would kind of prefer you do this 4 instruction group together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151396