RISC-V: Check the Zca extension when disabling the C extension using .option norvc.
Checks
Commit Message
.option norvc disables only the C extension, not all 16-bit instructions.
Signed-off-by: Ethan Yu-Cheng Liang <ycl669@andestech.com>
---
gas/config/tc-riscv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
2.45.2
CONFIDENTIALITY NOTICE:
This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation.
Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
Comments
Hi,
According to the newest ISA spec,
https://github.com/riscv/riscv-isa-manual/blob/main/src/zc.adoc, the C is
actually same as Zca, Zcf and Zcd, so this change looks conflict. The
original code should be correct, the RVC flag means enable the C extension,
so NORVC means disable C, and it makes sense to disable the RVC flag. But
this patch also reminds us that the current toolchain needs to be updated
to the newest spec.
1. C needs imply Zca, Zcf and Zcd, so the implicit table needs to be
updated.
2. Removing extensions using ".option arch" is more complicated than the
first implemented. For this case, C is the set of Zca, Zcf and Zcd, so we
need to imply those extensions when adding C, and should also need to
remove those extensions when removing C. That is - riscv_update_subset
(&riscv_rps_as, "-c,-zca,-zcf,-zcd") for .option norvc. But unfortunately
it's not always work to directly remove the extension from the implicit
table in reverse. For example, zcd implies d and zca, but remove zcd
doesn't mean remove d since zcd isn't a super set of d and zca.
cc Andrew in case I am wrong ;)
Thanks
Nelson
On Fri, Oct 4, 2024 at 3:12 PM Ethan Yu-Cheng Liang(梁淯程) <
ycl669@andestech.com> wrote:
> .option norvc disables only the C extension, not all 16-bit instructions.
>
> Signed-off-by: Ethan Yu-Cheng Liang <ycl669@andestech.com>
> ---
> gas/config/tc-riscv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index ef455e449b9..4ef7b8e689e 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -4917,7 +4917,7 @@ s_riscv_option (int x ATTRIBUTE_UNUSED)
> {
> riscv_update_subset (&riscv_rps_as, "-c");
> riscv_set_arch_str (&riscv_rps_as.subset_list->arch_str);
> - riscv_set_rvc (false);
> + riscv_set_rvc (riscv_subset_supports (&riscv_rps_as, "zca"));
> }
> else if (strcmp (name, "pic") == 0)
> riscv_opts.pic = true;
> --
> 2.45.2
>
> CONFIDENTIALITY NOTICE:
>
> This e-mail (and its attachments) may contain confidential and legally
> privileged information or information protected from disclosure. If you are
> not the intended recipient, you are hereby notified that any disclosure,
> copying, distribution, or use of the information contained herein is
> strictly prohibited. In this case, please immediately notify the sender by
> return e-mail, delete the message (and any accompanying documents) and
> destroy all printed hard copies. Thank you for your cooperation.
>
> Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
>
On Mon, Oct 7, 2024 at 11:32 PM Nelson Chu <nelson@rivosinc.com> wrote:
>
> Hi,
>
> According to the newest ISA spec, https://github.com/riscv/riscv-isa-manual/blob/main/src/zc.adoc, the C is actually same as Zca, Zcf and Zcd, so this change looks conflict. The original code should be correct, the RVC flag means enable the C extension, so NORVC means disable C, and it makes sense to disable the RVC flag. But this patch also reminds us that the current toolchain needs to be updated to the newest spec.
>
> 1. C needs imply Zca, Zcf and Zcd, so the implicit table needs to be updated.
>
> 2. Removing extensions using ".option arch" is more complicated than the first implemented. For this case, C is the set of Zca, Zcf and Zcd, so we need to imply those extensions when adding C, and should also need to remove those extensions when removing C. That is - riscv_update_subset (&riscv_rps_as, "-c,-zca,-zcf,-zcd") for .option norvc. But unfortunately it's not always work to directly remove the extension from the implicit table in reverse. For example, zcd implies d and zca, but remove zcd doesn't mean remove d since zcd isn't a super set of d and zca.
>
> cc Andrew in case I am wrong ;)
Nelson's right about the ISA implications.
".option rvc" is of course not defined by the ISA, and the ASM manual
doesn't do a great job of defining it, either. But the original
intent was to suppress automatic instruction compression (regardless
of exactly which extension it was). IOW, the intent was to make it so
that all instructions that are notionally 32 bits long remain 32 bits
long. Enabling or disabling extensions was not the intent.
>
> Thanks
> Nelson
>
>
> On Fri, Oct 4, 2024 at 3:12 PM Ethan Yu-Cheng Liang(梁淯程) <ycl669@andestech.com> wrote:
>>
>> .option norvc disables only the C extension, not all 16-bit instructions.
>>
>> Signed-off-by: Ethan Yu-Cheng Liang <ycl669@andestech.com>
>> ---
>> gas/config/tc-riscv.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> index ef455e449b9..4ef7b8e689e 100644
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -4917,7 +4917,7 @@ s_riscv_option (int x ATTRIBUTE_UNUSED)
>> {
>> riscv_update_subset (&riscv_rps_as, "-c");
>> riscv_set_arch_str (&riscv_rps_as.subset_list->arch_str);
>> - riscv_set_rvc (false);
>> + riscv_set_rvc (riscv_subset_supports (&riscv_rps_as, "zca"));
>> }
>> else if (strcmp (name, "pic") == 0)
>> riscv_opts.pic = true;
>> --
>> 2.45.2
>>
>> CONFIDENTIALITY NOTICE:
>>
>> This e-mail (and its attachments) may contain confidential and legally privileged information or information protected from disclosure. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein is strictly prohibited. In this case, please immediately notify the sender by return e-mail, delete the message (and any accompanying documents) and destroy all printed hard copies. Thank you for your cooperation.
>>
>> Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
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 ;)
Nelson
On Tue, Oct 8, 2024 at 2:41 PM Andrew Waterman <andrew@sifive.com> wrote:
> On Mon, Oct 7, 2024 at 11:32 PM Nelson Chu <nelson@rivosinc.com> wrote:
> >
> > Hi,
> >
> > According to the newest ISA spec,
> https://github.com/riscv/riscv-isa-manual/blob/main/src/zc.adoc, the C is
> actually same as Zca, Zcf and Zcd, so this change looks conflict. The
> original code should be correct, the RVC flag means enable the C extension,
> so NORVC means disable C, and it makes sense to disable the RVC flag. But
> this patch also reminds us that the current toolchain needs to be updated
> to the newest spec.
> >
> > 1. C needs imply Zca, Zcf and Zcd, so the implicit table needs to be
> updated.
> >
> > 2. Removing extensions using ".option arch" is more complicated than the
> first implemented. For this case, C is the set of Zca, Zcf and Zcd, so we
> need to imply those extensions when adding C, and should also need to
> remove those extensions when removing C. That is - riscv_update_subset
> (&riscv_rps_as, "-c,-zca,-zcf,-zcd") for .option norvc. But unfortunately
> it's not always work to directly remove the extension from the implicit
> table in reverse. For example, zcd implies d and zca, but remove zcd
> doesn't mean remove d since zcd isn't a super set of d and zca.
> >
> > cc Andrew in case I am wrong ;)
>
> Nelson's right about the ISA implications.
>
> ".option rvc" is of course not defined by the ISA, and the ASM manual
> doesn't do a great job of defining it, either. But the original
> intent was to suppress automatic instruction compression (regardless
> of exactly which extension it was). IOW, the intent was to make it so
> that all instructions that are notionally 32 bits long remain 32 bits
> long. Enabling or disabling extensions was not the intent.
>
> >
> > Thanks
> > Nelson
> >
> >
> > On Fri, Oct 4, 2024 at 3:12 PM Ethan Yu-Cheng Liang(梁淯程) <
> ycl669@andestech.com> wrote:
> >>
> >> .option norvc disables only the C extension, not all 16-bit
> instructions.
> >>
> >> Signed-off-by: Ethan Yu-Cheng Liang <ycl669@andestech.com>
> >> ---
> >> gas/config/tc-riscv.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> >> index ef455e449b9..4ef7b8e689e 100644
> >> --- a/gas/config/tc-riscv.c
> >> +++ b/gas/config/tc-riscv.c
> >> @@ -4917,7 +4917,7 @@ s_riscv_option (int x ATTRIBUTE_UNUSED)
> >> {
> >> riscv_update_subset (&riscv_rps_as, "-c");
> >> riscv_set_arch_str (&riscv_rps_as.subset_list->arch_str);
> >> - riscv_set_rvc (false);
> >> + riscv_set_rvc (riscv_subset_supports (&riscv_rps_as, "zca"));
> >> }
> >> else if (strcmp (name, "pic") == 0)
> >> riscv_opts.pic = true;
> >> --
> >> 2.45.2
> >>
> >> CONFIDENTIALITY NOTICE:
> >>
> >> This e-mail (and its attachments) may contain confidential and legally
> privileged information or information protected from disclosure. If you are
> not the intended recipient, you are hereby notified that any disclosure,
> copying, distribution, or use of the information contained herein is
> strictly prohibited. In this case, please immediately notify the sender by
> return e-mail, delete the message (and any accompanying documents) and
> destroy all printed hard copies. Thank you for your cooperation.
> >>
> >> Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
>
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
but a bit friendlier. I think that whole block probably needs a
rewrite, though...
> Nelson
>
>
> On Tue, Oct 8, 2024 at 2:41 PM Andrew Waterman <andrew@sifive.com> wrote:
>
>> On Mon, Oct 7, 2024 at 11:32 PM Nelson Chu <nelson@rivosinc.com> wrote:
>> >
>> > Hi,
>> >
>> > According to the newest ISA spec,
>> https://github.com/riscv/riscv-isa-manual/blob/main/src/zc.adoc, the C is
>> actually same as Zca, Zcf and Zcd, so this change looks conflict. The
>> original code should be correct, the RVC flag means enable the C extension,
>> so NORVC means disable C, and it makes sense to disable the RVC flag. But
>> this patch also reminds us that the current toolchain needs to be updated
>> to the newest spec.
>> >
>> > 1. C needs imply Zca, Zcf and Zcd, so the implicit table needs to be
>> updated.
>> >
>> > 2. Removing extensions using ".option arch" is more complicated than the
>> first implemented. For this case, C is the set of Zca, Zcf and Zcd, so we
>> need to imply those extensions when adding C, and should also need to
>> remove those extensions when removing C. That is - riscv_update_subset
>> (&riscv_rps_as, "-c,-zca,-zcf,-zcd") for .option norvc. But unfortunately
>> it's not always work to directly remove the extension from the implicit
>> table in reverse. For example, zcd implies d and zca, but remove zcd
>> doesn't mean remove d since zcd isn't a super set of d and zca.
>> >
>> > cc Andrew in case I am wrong ;)
>>
>> Nelson's right about the ISA implications.
>>
>> ".option rvc" is of course not defined by the ISA, and the ASM manual
>> doesn't do a great job of defining it, either. But the original
>> intent was to suppress automatic instruction compression (regardless
>> of exactly which extension it was). IOW, the intent was to make it so
>> that all instructions that are notionally 32 bits long remain 32 bits
>> long. Enabling or disabling extensions was not the intent.
>>
>> >
>> > Thanks
>> > Nelson
>> >
>> >
>> > On Fri, Oct 4, 2024 at 3:12 PM Ethan Yu-Cheng Liang(梁淯程) <
>> ycl669@andestech.com> wrote:
>> >>
>> >> .option norvc disables only the C extension, not all 16-bit
>> instructions.
>> >>
>> >> Signed-off-by: Ethan Yu-Cheng Liang <ycl669@andestech.com>
>> >> ---
>> >> gas/config/tc-riscv.c | 2 +-
>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> >> index ef455e449b9..4ef7b8e689e 100644
>> >> --- a/gas/config/tc-riscv.c
>> >> +++ b/gas/config/tc-riscv.c
>> >> @@ -4917,7 +4917,7 @@ s_riscv_option (int x ATTRIBUTE_UNUSED)
>> >> {
>> >> riscv_update_subset (&riscv_rps_as, "-c");
>> >> riscv_set_arch_str (&riscv_rps_as.subset_list->arch_str);
>> >> - riscv_set_rvc (false);
>> >> + riscv_set_rvc (riscv_subset_supports (&riscv_rps_as, "zca"));
>> >> }
>> >> else if (strcmp (name, "pic") == 0)
>> >> riscv_opts.pic = true;
>> >> --
>> >> 2.45.2
>> >>
>> >> CONFIDENTIALITY NOTICE:
>> >>
>> >> This e-mail (and its attachments) may contain confidential and legally
>> privileged information or information protected from disclosure. If you are
>> not the intended recipient, you are hereby notified that any disclosure,
>> copying, distribution, or use of the information contained herein is
>> strictly prohibited. In this case, please immediately notify the sender by
>> return e-mail, delete the message (and any accompanying documents) and
>> destroy all printed hard copies. Thank you for your cooperation.
>> >>
>> >> Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
>>
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...
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
>
> but a bit friendlier. I think that whole block probably needs a
> rewrite, though...
>
> > Nelson
> >
> >
> > On Tue, Oct 8, 2024 at 2:41 PM Andrew Waterman <andrew@sifive.com> wrote:
> >
> >> On Mon, Oct 7, 2024 at 11:32 PM Nelson Chu <nelson@rivosinc.com> wrote:
> >> >
> >> > Hi,
> >> >
> >> > According to the newest ISA spec,
> >> https://github.com/riscv/riscv-isa-manual/blob/main/src/zc.adoc, the C is
> >> actually same as Zca, Zcf and Zcd, so this change looks conflict. The
> >> original code should be correct, the RVC flag means enable the C extension,
> >> so NORVC means disable C, and it makes sense to disable the RVC flag. But
> >> this patch also reminds us that the current toolchain needs to be updated
> >> to the newest spec.
> >> >
> >> > 1. C needs imply Zca, Zcf and Zcd, so the implicit table needs to be
> >> updated.
> >> >
> >> > 2. Removing extensions using ".option arch" is more complicated than the
> >> first implemented. For this case, C is the set of Zca, Zcf and Zcd, so we
> >> need to imply those extensions when adding C, and should also need to
> >> remove those extensions when removing C. That is - riscv_update_subset
> >> (&riscv_rps_as, "-c,-zca,-zcf,-zcd") for .option norvc. But unfortunately
> >> it's not always work to directly remove the extension from the implicit
> >> table in reverse. For example, zcd implies d and zca, but remove zcd
> >> doesn't mean remove d since zcd isn't a super set of d and zca.
> >> >
> >> > cc Andrew in case I am wrong ;)
> >>
> >> Nelson's right about the ISA implications.
> >>
> >> ".option rvc" is of course not defined by the ISA, and the ASM manual
> >> doesn't do a great job of defining it, either. But the original
> >> intent was to suppress automatic instruction compression (regardless
> >> of exactly which extension it was). IOW, the intent was to make it so
> >> that all instructions that are notionally 32 bits long remain 32 bits
> >> long. Enabling or disabling extensions was not the intent.
> >>
> >> >
> >> > Thanks
> >> > Nelson
> >> >
> >> >
> >> > On Fri, Oct 4, 2024 at 3:12 PM Ethan Yu-Cheng Liang(梁淯程) <
> >> ycl669@andestech.com> wrote:
> >> >>
> >> >> .option norvc disables only the C extension, not all 16-bit
> >> instructions.
> >> >>
> >> >> Signed-off-by: Ethan Yu-Cheng Liang <ycl669@andestech.com>
> >> >> ---
> >> >> gas/config/tc-riscv.c | 2 +-
> >> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> >> >> index ef455e449b9..4ef7b8e689e 100644
> >> >> --- a/gas/config/tc-riscv.c
> >> >> +++ b/gas/config/tc-riscv.c
> >> >> @@ -4917,7 +4917,7 @@ s_riscv_option (int x ATTRIBUTE_UNUSED)
> >> >> {
> >> >> riscv_update_subset (&riscv_rps_as, "-c");
> >> >> riscv_set_arch_str (&riscv_rps_as.subset_list->arch_str);
> >> >> - riscv_set_rvc (false);
> >> >> + riscv_set_rvc (riscv_subset_supports (&riscv_rps_as, "zca"));
> >> >> }
> >> >> else if (strcmp (name, "pic") == 0)
> >> >> riscv_opts.pic = true;
> >> >> --
> >> >> 2.45.2
> >> >>
> >> >> CONFIDENTIALITY NOTICE:
> >> >>
> >> >> This e-mail (and its attachments) may contain confidential and legally
> >> privileged information or information protected from disclosure. If you are
> >> not the intended recipient, you are hereby notified that any disclosure,
> >> copying, distribution, or use of the information contained herein is
> >> strictly prohibited. In this case, please immediately notify the sender by
> >> return e-mail, delete the message (and any accompanying documents) and
> >> destroy all printed hard copies. Thank you for your cooperation.
> >> >>
> >> >> Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
> >>
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".
Jan
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
@@ -4917,7 +4917,7 @@ s_riscv_option (int x ATTRIBUTE_UNUSED)
{
riscv_update_subset (&riscv_rps_as, "-c");
riscv_set_arch_str (&riscv_rps_as.subset_list->arch_str);
- riscv_set_rvc (false);
+ riscv_set_rvc (riscv_subset_supports (&riscv_rps_as, "zca"));
}
else if (strcmp (name, "pic") == 0)
riscv_opts.pic = true;