Re: [PATCH 0/3] RISC-V: Fix CSR accessibility and implications

[Nelson Chu <nelson@rivosinc.com>]

On Wed, Sep 7, 2022 at 1:54 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>
> Tracker on GitHub:
> <https://github.com/a4lg/binutils-gdb/wiki/riscv_fix_csr_access_and_imply>
>
> While I'm reorganizing "b-ext*" and "k-ext*" tests (to make sure that they
> are able to be removed since there are neither 'B' nor 'K' extensions),
> I found something interesting.
>
> If we assemble a file with "csrr a0, seed" (uses "seed" CSR from the 'Zkr'
> extension) with "-march=rv32i_zkr", it causes an error to require 'Zicsr'.
> Yes, current GNU Binutils does not imply 'Zicsr' from 'Zkr' so that adding
> 'Zkr' extension alone is not enough to access corresponding CSRs
> (current version requires an option like "-march=rv32i_zicsr_zkr").
> "k-ext.d" does not give 'Zicsr' either (and does not test the seed CSR) and
> adding the line "csrr a0, seed" to this test file resulted the same.

According to the ISA spec, Zicsr is the sub-extension of i, and is
used to control the csr instructions, so I don't see any conflict for
the current toolchain.  Though it looks weird that we allow csrs but
need to enable csr instruction by another extension.  If you still
have problems, you can create and raise an issue maybe in
riscv-isa-manual or riscv-v-spec, to clarify if the zicsr controls
both csr and csr instructions.  Maybe it's worth clarifying this,
since LLVM doesn't recognize zicsr in the main branch.

> That means something.  If an extension adds CSR(s), we should imply 'Zicsr'
> from that extension (just like 'F').  So, we have to imply 'Zicsr' from
> following extensions:
>
> -   'H'
> -   'Zkr'
> -   'Zve32x' (I'll talk later)
> -   'Smstateen'
> -   'Sscofpmf'
> -   'Sstc'
>
> 'Zkr' is fixed in PATCH 2/3 and 'H' and 'S*' are fixed in PATCH 3/3.
> The only remaining extension is 'Zve32x' but I need to talk more to explain
> actual changes (specific to vectors) in PATCH 1/3.
>
> On the current version of GNU Binutils, CSRs with CSR_CLASS_V means they
> require the 'V' extension.  However, there are a few vector subextensions
> that implement vector subsets (intended for embedded processors).

Check zve32x for vcsr should be enough, since LLVM looks to did the
similar thing for vector instructions,
https://github.com/llvm/llvm-project/blob/7167a4207ee2c07cb192da1788f919332f83b456/llvm/lib/Support/RISCVISAInfo.cpp#L708

> -   'Zve64d' (superset of 'Zve64f')
> -   'Zve64f' (superset of 'Zve32f' and 'Zve64x')
> -   'Zve64x' (superset of 'Zve32x')
> -   'Zve32f' (superset of 'Zve32x')
> -   'Zve32x'
>
> : Graph: Dependency graph of some vector/FP extensions
> :
> : +-------> D --------> F -----> Zicsr
> : |         ^           ^
> : |         |           |
> : V ---> Zve64d ---> Zve64f ---> Zve64x
> :                       |           |
> :                       V           V
> :                    Zve32f ---> Zve32x
> :                                   |
> :                                   |
> :                                   +---> (Zicsr [added in PATCH 1/3])
>
> They also require general purpose vector CSRs (vstart, vl, vtype and vlenb).
> So, corresponding CSR_CLASS_V with the 'V' extension is inappropriate
> (they should require 'Zve32x' instead, the minimum vector subset).
>
> Remaining CSRs are:
>
> -   vxsat
> -   vxrm
> -   vcsr
>
> They are related to fixed-point arithmetic and 18.2 "Zve*: Vector Extensions
> for Embedded Processors" says:
>
> > All Zve* extensions support all vector fixed-point arithmetic instructions
> > (Vector Fixed-Point Arithmetic Instructions), except that vsmul.vv and
> > vsmul.vx are not supported for EEW=64 in Zve64*.
>
> So, their minimum requirement shall be also 'Zve32x', not 'V'.
>
> As a consequence, we can conclude that changing requirements of CSR_CLASS_V
> from 'V' to 'Zve32x' is sufficient to avoid CSR accessibility warnings.
>
> Also, 'Zve32x' must imply 'Zicsr' (just like the rest) to avoid CSR
> instruction errors.  This is only effective on 'Zve32x' and 'Zve64x'
> because, for instance, 'Zve32f' implies 'F' and 'F' implies 'Zicsr'
> (see the graph above).
>
> I didn't rename CSR_CLASS_V to CSR_CLASS_ZVE32X because the name gets
> difficult and there's already INSN_CLASS_V (effectively requires 'Zve32x'
> with some exceptions).
>
>
>
>
> Tsukasa OI (3):
>   RISC-V: Fix vector CSR requirements and imply
>   RISC-V: Imply 'Zicsr' from 'Zkr'
>   RISC-V: Imply 'Zicsr' from some privileged extensions
>
>  bfd/elfxx-riscv.c                            |  6 +++++
>  gas/config/tc-riscv.c                        |  2 +-
>  gas/testsuite/gas/riscv/csr-version-1p10.l   | 28 ++++++++++----------
>  gas/testsuite/gas/riscv/csr-version-1p11.l   | 28 ++++++++++----------
>  gas/testsuite/gas/riscv/csr-version-1p12.l   | 28 ++++++++++----------
>  gas/testsuite/gas/riscv/csr-version-1p9p1.l  | 28 ++++++++++----------
>  gas/testsuite/gas/riscv/march-imply-h.d      |  6 +++++
>  gas/testsuite/gas/riscv/vector-csrs-zve32f.d | 21 +++++++++++++++
>  gas/testsuite/gas/riscv/vector-csrs-zve32x.d | 21 +++++++++++++++
>  gas/testsuite/gas/riscv/vector-csrs-zve64d.d | 21 +++++++++++++++
>  gas/testsuite/gas/riscv/vector-csrs-zve64f.d | 21 +++++++++++++++
>  gas/testsuite/gas/riscv/vector-csrs-zve64x.d | 21 +++++++++++++++
>  gas/testsuite/gas/riscv/vector-csrs.s        | 12 +++++++++
>  gas/testsuite/gas/riscv/zkr.d                | 10 +++++++
>  gas/testsuite/gas/riscv/zkr.s                |  2 ++
>  15 files changed, 198 insertions(+), 57 deletions(-)
>  create mode 100644 gas/testsuite/gas/riscv/march-imply-h.d
>  create mode 100644 gas/testsuite/gas/riscv/vector-csrs-zve32f.d
>  create mode 100644 gas/testsuite/gas/riscv/vector-csrs-zve32x.d
>  create mode 100644 gas/testsuite/gas/riscv/vector-csrs-zve64d.d
>  create mode 100644 gas/testsuite/gas/riscv/vector-csrs-zve64f.d
>  create mode 100644 gas/testsuite/gas/riscv/vector-csrs-zve64x.d
>  create mode 100644 gas/testsuite/gas/riscv/vector-csrs.s
>  create mode 100644 gas/testsuite/gas/riscv/zkr.d
>  create mode 100644 gas/testsuite/gas/riscv/zkr.s
>
>
> base-commit: f555b327d41ed72ffae28caae550f5f86312db43
> --
> 2.34.1
>