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

[Tsukasa OI <research_trasio@irq.a4lg.com>]

On 2022/09/07 17:49, Nelson Chu wrote:
> 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.

I remember I asked something similar to Andrew Waterman already but I
forgot where and when.  I'll figure it out later.

> 
>> 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

I agree and that's what a part of PATCH 1/3 is doing.  It looks changing
vector CSR requirement from 'V' to 'Zve32x' is acceptable to you.  If
so, I'll submit separate patch for this part.

Thanks,
Tsukasa

> 
>> -   '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
>>
>