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

Fri Feb 14 08:50:39 PST 2020

fpallares added a comment.

Thanks for addressing my comments @HsiangKai. After a closer look I've added some more inline comments, and some general ones too:

- We are missing tests on whole register move instructions `vmv<nf>r.v`.
- Overlap diagnosis for those instructions that are missing the register overlap constraints (see inline comments) should be added in `RISCVAsmParser::validateInstruction`, as well as new tests.
- I've noticed you are using `${vm}` for describing the mask arg on most instructions, but AFAICT `$vm` is equally valid (e.g. You could write `$vd, $vs2$vm` instead of `$vd, $vs2${vm}`).
- We should probably recognize the assembler pseudo instructions defined in the ISA. Most can be implemented via `InstAlias`. I suggest to do this in another patch.
- Instruction from the `Zvqmac` extension instructions were added, should we add instructions for the other extensions as well? Otherwise we might want to restrict this patch to the base only.

================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:154

+static DecodeStatus DecodeVRRegisterClass(MCInst &Inst, uint64_t RegNo,
+                                          uint64_t Address,
----------------
Following @evandro's  naming suggestions, this should probably be renamed to `DecodeVRegisterClass`.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:39
+
+def VRegOp: RegisterOperand<VR> {
+  let ParserMatchClass = VRegAsmOperand;
----------------
Whitespace missing before the colon.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:54
+
+def VMaskOp: RegisterOperand<VMV0> {
+  let ParserMatchClass = VMaskAsmOperand;
----------------
Ditto.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:106
+
+let hasSideEffects = 1, mayLoad = 0, mayStore = 1 in {
+// store vd, vs3, (rs1), vm
----------------
I don't see why setting `hasSideEffects` is necessary and setting `mayStore` is not enough, could you please elaborate a bit on this?

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:425
+defm VSUB_V : VALU_IV_V_X<"vsub", 0b000010>;
+defm VRSUB_V: VALU_IV_X_I<"vrsub", 0b000011>;
+
----------------
Whitespace missing before the colon.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:437
+defm VWADD_W : VALU_MV_V_X<"vwadd", 0b110101, "w">;
+defm VWSUB_W : VALU_MV_V_X<"vwsub", 0b110111, "w">;
+
----------------
I'd say those instructions need the `@earlyclobber $vd` restriction too. Not because of the first source operand (since it has the same width as the destination), but because of the second (in the case of `VWOP_WV`), or the mask (for `VWOP_WX`).

Quoting section [[ https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#112-widening-vector-arithmetic-instructions | 11.2. Widening Vector Arithmetic Instructions ]]:
> The destination vector register group cannot overlap a source vector register group of a different element width (including the mask register if masked), otherwise an illegal instruction exception is raised.

This has the downside that the `earlyclobber` constraint is too coarse and will impose unnecessary restrictions by not allowing the destination to overlap with the first (wide) operand, even when the instruction is used unmasked. Maybe we can use the `earlyclobber` for now, and find a better solution for this in a later patch.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:459
+defm VNSRL_W : VALU_IV_V_X_I<"vnsrl", 0b101100, uimm5, "w">;
+defm VNSRA_W : VALU_IV_V_X_I<"vnsra", 0b101101, uimm5, "w">;
+
----------------
In section [[ https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#113-narrowing-vector-arithmetic-instructions | 11.3. Narrowing Vector Arithmetic Instructions ]] we find the following paragraph:

> The destination vector register group cannot overlap the first source vector register group (specified by vs2). The destination vector register group cannot overlap the mask register if used, unless LMUL=1. If either constraint is violated, an illegal instruction exception is raised.

>From my understanding, this applies to `VNSRL` and `VNSRA`. In that case we need an `@earlyclobber` constraint here.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:479
+defm VMUL_V : VALU_MV_V_X<"vmul", 0b100101>;
+defm VMULH_V: VALU_MV_V_X<"vmulh", 0b100111>;
+defm VMULHU_V : VALU_MV_V_X<"vmulhu", 0b100100>;
----------------
Whitespace missing before the colon.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:556
+defm VNCLIPU_W : VALU_IV_V_X_I<"vnclipu", 0b101110, uimm5, "w">;
+defm VNCLIP_W : VALU_IV_V_X_I<"vnclip", 0b101111, uimm5, "w">;
+
----------------
I'd say overlap restrictions on narrowing instructions apply to `VNCLIP` and `VNCLIPU` too.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:570
+defm VFWADD_W : VALU_FV_V_F<"vfwadd", 0b110100, "w">;
+defm VFWSUB_W : VALU_FV_V_F<"vfwsub", 0b110110, "w">;
+
----------------
As discussed in another inline comment above (for `VWADD_W` and such), there is an overlap restriction for these instructions too.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:583
+// Vector Single-Width Floating-Point Fused Multiply-Add Instructions
+defm VFMACC_V: VALUr_FV_V_F<"vfmacc", 0b101100>;
+defm VFNMACC_V : VALUr_FV_V_F<"vfnmacc", 0b101101>;
----------------
Whitespace missing before the colon.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:660
+defm VFNCVT_F_F_W : VALU_FV_VS2<"vfncvt.f.f.w", 0b100010, 0b10100>;
+defm VFNCVT_ROD_F_F_W : VALU_FV_VS2<"vfncvt.rod.f.f.w", 0b100010, 0b10101>;
+
----------------
I believe we are also missing the `@earlyclobber $vd` overlap constraints for these narrowing conversions.

>From section [[ https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#1417-narrowing-floating-pointinteger-type-convert-instructions | 14.17. Narrowing Floating-Point/Integer Type-Convert Instructions ]]:
> These instructions have the same constraints on vector register overlap as other narrowing instructions (see [[ https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#sec-narrowing | Narrowing Vector Arithmetic Instructions ]]).

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:674
+defm VWREDSUMU : VALU_IV_V<"vwredsumu", 0b110000>;
+defm VWREDSUM : VALU_IV_V<"vwredsum", 0b110001>;
+
----------------
If I'm not mistaken, those instructions should have the `@earlyclobber $vd` constraint too to avoid overlaps.

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:684
+defm VFWREDOSUM : VALU_FV_V<"vfwredosum", 0b110011>;
+defm VFWREDSUM : VALU_FV_V<"vfwredsum", 0b110001>;
+
----------------
Ditto.

================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:50
+def sub_vrm8    : SubRegIndex<256>;
+def sub_vrm8_hi : SubRegIndex<256, 256>;
 } // Namespace = "RISCV"
----------------
My understanding is that here you specify minimum sizes (at least we will have one element of size `ELEN`). As they stand, they are consistent with `ELEN=64`. So far it is unclear to me what sizes we would want to use if we also want to support `ELEN=32`. I imagine we could reuse `HwMode` or something similar. 

As far as the offsets are concerned I'd suggest setting them to `-1` because we don't really know the offset where the `hi` part starts.

That said, you already mentioned that types are not that important in this patch.

================
Comment at: llvm/test/MC/RISCV/rvv/macc.s:1
+// RUN: llvm-mc -triple=riscv64 -show-encoding -mattr=+v -mattr=+vqmac < %s \
+// RUN:        | FileCheck %s --check-prefixes=CHECK-ENCODING,CHECK-INST
----------------
Looks like `+v` attribute gets passed twice. I'm also not sure we need `+a` and `+c`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69987