[PATCH] RISC-V: Check the Zca extension when disabling the C extension using .option norvc.

[Palmer Dabbelt]

On Tue, 08 Oct 2024 23:27:16 PDT (-0700), jbeulich@suse.com wrote:
> On 09.10.2024 03:04, Kito Cheng wrote:
>> I originally thought we shouldn't change the semantics or behavior,
>> just toggle the C extension, but I quickly realized that wouldn't
>> work—it would make .option rvc a nop. That's because now C implies Zca
>> and Zcf/Zcd (if f/d are present). For example, after using `.option
>> rvc` with rv64g, it becomes rv64gc_zca_zcd, and after `.option norvc`,
>> it turns into rv64g_zca_zcd, which is basically still the same as
>> rv64gc_zca_zcd...
>
> Which is wrong anyway, isn't it? To me even rv64gc_zca_zcd looks wrong
> (as in: not backwards compatible); it ought to be just rv64gc. Perhaps
> thinking of the relationship between C and Zca/Zcf/Zcd as "implies" is
> wrong, and ...
>
>> So I think changing the documentation is the only solution, and
>> turning off all 16-bit instructions is probably what most users would
>> expect.
>>
>> On Wed, Oct 9, 2024 at 8:19 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>>
>>> On Tue, 08 Oct 2024 00:34:43 PDT (-0700), Nelson Chu wrote:
>>>> Yeah thanks for the clarification.  The .option norvc intents to make all
>>>> instructions are 32 bits, although removing extensions is not the original
>>>> intention, but for the correctness probably also needs to remove all 16-bit
>>>> extensions, including the new zcmop/zcmt/zcmp?
>>>>
>>>> However, I prefer to keep the original intention of ".option rvc/norvc",
>>>> which means allow/dis-allow 16-bit instructions, so that probably not same
>>>> as "allow/dis-allow the standard C extension" since we will/already ratify
>>>> more 16-bit ZC extensions, which aren't included in standard C.  That
>>>> should also works for RVC flags since it means no 16 bits instructions if
>>>> the flag isn't raised.  Therefore,
>>>>
>>>> .option rvc = +c,+zcmop,+zcmt,+zcmp, ...
>>>> .option norvc = -c,-zcmop,-zcmt,-zcmt, ...
>>>>
>>>> cc more people, Jim, Palmer and Kito ;)
>>>
>>> I don't really know if we can say much about the original intent, this
>>> all comes back from the time before we had all these sub-extensions so
>>> we really just hadn't planned for any of this complexity (hence it
>>> breaking a lot of the early design).
>>>
>>> Having `.option norvc` disable all 16-bit instruction does seem like
>>> what users would want, though -- otherwise we end up in an odd spot
>>> where there's no straight-forward way to disable opportunistic
>>> compression, which will be a headache.  Unless I'm missing something we
>>> already disable all 16-bit instructions under `.option norvc`, so we're
>>> safe on the implementation side here.
>>>
>>> Unfortunately the RISC-V ASM manual docs are pretty clear that `.option
>>> norvc` disables the C extension
>>>
>>>     == `.option`
>>>
>>>     === `rvc`/`norvc`
>>>
>>>     This option will be deprecated soon after `.option arch` has been widely
>>>     implemented on main stream open source toolchains.
>>>
>>>     Enable/disable the C-extension for the following code region. This option is
>>>     equivalent to `.option arch, +c`/`.option arch, -c`, but widely supported by
>>>     older toolchain versions.
>>>
>>> which doesn't match that behavior.  I'm generally fine just ignoring
>>> these RISC-V software specs where they don't make sense, though, as they
>>> tend to be full of wacky stuff that we already ignore all over the
>>> place.
>>>
>>> The binutils docs are a bit vague
>>>
>>>     @item rvc
>>>     @itemx norvc
>>>     Enables or disables the generation of compressed instructions.  Instructions
>>>     are opportunistically compressed by the RISC-V assembler when possible, but
>>>     sometimes this behavior is not desirable, especially when handling alignments.
>>>
>>> but I think we can just call something like
>>>
>>>     diff --git a/gas/doc/c-riscv.texi b/gas/doc/c-riscv.texi
>>>     index 7484a71798a..45b9617b6b9 100644
>>>     --- a/gas/doc/c-riscv.texi
>>>     +++ b/gas/doc/c-riscv.texi
>>>     @@ -189,7 +189,7 @@ command-line options are respected for the bulk of the file being assembled.
>>>
>>>      @item rvc
>>>      @itemx norvc
>>>     -Enables or disables the generation of compressed instructions.  Instructions
>>>     +Enables or disables the generation of 16-bit instructions.  Instructions
>>>      are opportunistically compressed by the RISC-V assembler when possible, but
>>>      sometimes this behavior is not desirable, especially when handling alignments.
>>>
>>> a fix to make the docs match the implementation.  I guess the name is a
>>> bit clunky now, but I don't think that's such a big deal.
>>>
>>> Looks like we also have an odditiy here where `.option arch, -c` doesn't
>>> disable Zca if it's been explicitly turned on.  I'm not sure what we
>>> want to do there.
>>>
>>> We probably also want something along the lines of
>>>
>>>     diff --git a/gas/doc/c-riscv.texi b/gas/doc/c-riscv.texi
>>>     index 7484a71798a..870ade20582 100644
>>>     --- a/gas/doc/c-riscv.texi
>>>     +++ b/gas/doc/c-riscv.texi
>>>     @@ -214,7 +214,7 @@ Enables or disables the CSR checking.
>>>      Enables or disables the extensions for specific code region.  For example,
>>>      @samp{.option arch, +m2p0} means add m extension with version 2.0, and
>>>      @samp{.option arch, -f, -d} means remove extensions, f and d, from the
>>>     -architecture string.  Note that, @samp{.option arch, +c, -c} have the same
>>>     +architecture string.  Note that, @samp{.option arch, +c, -c} doesn't have the same
>>>      behavior as @samp{.option rvc, norvc}.  However, they are also undesirable
>>>      sometimes.  Besides, @samp{.option arch, -i} is illegal, since we cannot
>>>      remove the base i extension anytime.  If you want to reset the whole ISA
>
> ... the original text here actually ought to be kept (and the implementation
> made match, if it doesn't already): If you look at things as C being the
> combination of Zca, Zcf, and Zcd (with the F/D dependencies left aside), i.e.
> not "implies" but "includes", ".option arch, -c" ought to be disabling
> everything C covers (the three named finer grained extensions) and everything
> C is a prereq for (other Zc*). I.e. no different from "option norvc".

This just popped up in GCC as well: 
https://inbox.sourceware.org/gcc-patches/61281588-aec6-416b-8fa7-ac37d46697e5@gmail.com/ 
.  I'v basically given up on trying to figure out these sort of detials 
in RISC-V land, so I'm fine with either behavior.  Just as long as the 
docs match the implementation ;)

> Jan