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

[Hsiangkai Wang via Phabricator via llvm-commits]

HsiangKai marked 3 inline comments as done.
HsiangKai added inline comments.


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:154
 
+static DecodeStatus DecodeVRRegisterClass(MCInst &Inst, uint64_t RegNo,
+                                          uint64_t Address,
----------------
fpallares wrote:
> Following @evandro's  naming suggestions, this should probably be renamed to `DecodeVRegisterClass`.
The function name will be expected by TableGen as `"Decode" + Record->getName().str() + "RegisterClass"`. It seems related to the name of register class. Currently, the name of RegisterClass for V-extension registers is "VR".


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:106
+
+let hasSideEffects = 1, mayLoad = 0, mayStore = 1 in {
+// store vd, vs3, (rs1), vm
----------------
evandro wrote:
> fpallares wrote:
> > I don't see why setting `hasSideEffects` is necessary and setting `mayStore` is not enough, could you please elaborate a bit on this?
> `hasSideEffects` is not necessary, since `mayStore` is indeed enough.  The purpose of `hasSideEffects` is as a catch all when the rest of the instruction attributes miss some side effect.
There is no default value for hasSideEffects, mayLoad, and mayStore in `Instruction` class. So, I specify these three values for V instructions. It follows the style in RISCVInstrInfo.td. I agree that we could give these attributes some default values in some base class for V instructions. I will try to refactor it.


================
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">;
+
----------------
fpallares wrote:
> 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.
In current implementation for MC layer, there is no LMUL information in these instructions. So, if LMUL=1, there is no such restriction for them. I have a downstream patch to extend these instruction definitions for LMUL > 1. Maybe we could keep this MC implementation for LMUL=1 and extend it later. I will add comments for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69987