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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
