[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Simon Cook via Phabricator via cfe-commits]

simoncook added a reviewer: simoncook.
simoncook added a comment.

This is looking good, overall the patch is nicely laid out which has made it easy to compare against the spec.

I've made a few comments, mostly about ordering of instructions so that they are identical to the spec.

One question, I appreciate the Zvediv/Zvqmac sections say they are not planned to be part of the base V extension, but is it worth having them too behind separate flags so they can be used/evaluated. (I'm thinking like the Zbproposedc extension we added for bitmanip, which is not part of the proposed finalized 'B', but is included for comparisons).

The other thing I've noticed looking through the spec, there are references to psuedoinstructions, but these aren't covered by this patch. It would be good for `InstAlias`es to be defined for these and land in the same patch.



================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:160
+
+  Register Reg = RISCV::V0 + RegNo;
+  Inst.addOperand(MCOperand::createReg(Reg));
----------------
If you are going to rely on the register names being contiguous, extend the static_asserts in RISCVRegisterInfo.cpp to validate this at compile time.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:449
+}
+}
+
----------------
Add comment noting end of earlyclobber/constraint scope


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:532
+// Vector Integer Move Instructions
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
+// op vd, vs1
----------------
I'm not sure if this will read better if you move the common `let vs2=0, vm=1` up here?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:551
+}
+}
+
----------------
Add `// hasSideEffects = 0, mayLoad = 0, mayStore = 0`


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:578
+
+// Vector Single-Width Integer Reduction Instructions
+defm VREDSUM : VALU_MV_V<"vredsum", 0b000000>;
----------------
Following through the 0.8 spec I would have expected the `defm VFADD_V : VALU_FV_V_F` result here, since the previous instruction was defined in section 13.5, and VFADD.V[VF] is the next thing defined in 14.2.

The chapter 14 instructions look like they are defined in the order the spec says (with the exception of VFREDOSUM, etc at the bottom of this file). Could you move chapter 13 instructions here so the order matches, to make it easier to validate as the spec advances.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:648
+
+}
+
----------------
The vfmv definition from line 814 should move here? (17.2?)


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:651
+foreach nf = [1, 2, 4, 8] in {
+  def VMV#nf#R_V  : RVInstV<0b100111, !add(nf, -1), OPIVI, (outs VRegOp:$vd),
+                            (ins VRegOp:$vs2), "vmv" # nf # "r.v",
----------------
This should be below vcompress?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69987