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

[Evandro Menezes via Phabricator via llvm-commits]

evandro added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:54
+    : SubtargetFeature<"v", "HasStdExtV", "true",
+                       "'V' (Vector Instructions)">;
+def HasStdExtV : Predicate<"Subtarget->hasStdExtV()">,
----------------
HsiangKai wrote:
> evandro wrote:
> > Should it also imply `FeatureStdExtD`?
> I didn't find that V will imply D extension. Could you indicate the description in spec about this?
Indeed it doesn't:

> If the base scalar ISA does not include floating-point, then a `fcsr` register is also added to hold mirrors of the `vxsat` and `vxrm` CSRs as explained below.

Which means that the instructions that take FP scalars should also depend on `hasStdExt{F|D}`.  Likewise when the scalar is a 64 bit integer, `hasRV64`.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:77
+
+let hasSideEffects = 0, mayLoad = 1, mayStore = 0 in {
+// load vd, (rs1), vm
----------------
HsiangKai wrote:
> evandro wrote:
> > No need to set fields to the default `0`.
> There is no default value for hasSideEffects, mayLoad, and mayStore.
Right.  It would be cleaner if they were moved to the respective base instruction class then.  Just a suggestion.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:534
+
+// Vector Fixed-Point Arithmetic Instructions
+defm VSADDU_V : VALU_IV_V_X_I<"vsaddu", 0b100000>;
----------------
HsiangKai wrote:
> evandro wrote:
> > Shouldn't these have `vxsat` in `Defs`?
> It should have no impact on CodeGen. It seems no instructions to use vxsat.
For the sake of completeness.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:106
+
+let hasSideEffects = 1, mayLoad = 0, mayStore = 1 in {
+// store vd, vs3, (rs1), vm
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69987