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

[Hsiangkai Wang via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:54
+    : SubtargetFeature<"v", "HasStdExtV", "true",
+                       "'V' (Vector Instructions)">;
+def HasStdExtV : Predicate<"Subtarget->hasStdExtV()">,
----------------
evandro wrote:
> rogfer01 wrote:
> > evandro wrote:
> > > 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`.
> > I don't think we have to do anything special with 64 bit: section 11.1 states what is the behaviour when `XLEN` is different than `SEW`.
> Good point. 👍🏻
Yes, it is a problem about accessing fcsr if F extension is turned off. However, I found that there is vcsr in the latest version.

https://github.com/riscv/riscv-v-spec/commit/b25b643b9c29b4fde570293c64ca682a99a09f2a

So, I think we could keep V and F as separate unrelated extensions.

I will add predicates for vector floating point operations.


================
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">;
+
----------------
rogfer01 wrote:
> HsiangKai wrote:
> > 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.
> Perhaps the wording in the spec is unclear here?
> 
> Looks like the following sentence does apply regardless of LMUL.
> 
> >> The destination vector register group cannot overlap the first source vector register group (specified by vs2).  
> 
> While the sentence that follows it is clear that applies only to LMUL>1
> 
> >> The destination vector register group cannot overlap the mask register if used, unless LMUL=1
> 
> You also said that
> 
> > I have a downstream patch to extend these instruction definitions for LMUL > 1
> 
> I'm curious here: did you use a Pseudo for that or somehow avoided the issue that they'd be encoded in the same way? 
> 
> Thanks!
I misunderstood the statements. Sorry for that. I will add earlyclobber for narrowing instructions. Thanks a lot.

About instruction definitions for LMUL > 1, I enable 'isCodeGenOnly' for these instructions to avoid encoding checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69987