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

[Ferran Pallarès Roca via Phabricator via llvm-commits]

fpallares marked an inline comment as done.
fpallares added inline comments.


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:154
 
+static DecodeStatus DecodeVRRegisterClass(MCInst &Inst, uint64_t RegNo,
+                                          uint64_t Address,
----------------
HsiangKai wrote:
> 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".
I see, sorry for the noise.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:106
+
+let hasSideEffects = 1, mayLoad = 0, mayStore = 1 in {
+// store vd, vs3, (rs1), vm
----------------
HsiangKai wrote:
> 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.
I was only referring tot he fact that `hasSideEffects` here changed from `0` to `1` in a recent update of the patch. I don't really think a refactor is necessary here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69987