diff mbox series

RISC-V: Checking 'm' extension when using RVV.

Message ID 20250121152332.797970-1-jiawei@iscas.ac.cn
State New
Headers
Series RISC-V: Checking 'm' extension when using RVV. |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Jiawei Jan. 21, 2025, 3:23 p.m. UTC 
  This patch add a new extension check when using `v` extension, it
currently should also enable `m` extension to make sure vector length
caculate correct.

bfd/ChangeLog:

	* elfxx-riscv.c (riscv_parse_check_conflicts): New check.

gas/ChangeLog:

	* testsuite/gas/riscv/march-fail-rv64i_v.d: New test.
	* testsuite/gas/riscv/march-fail-rv64i_v.l: New test.

---
 bfd/elfxx-riscv.c                            | 10 +++++++++-
 gas/testsuite/gas/riscv/march-fail-rv64i_v.d |  4 ++++
 gas/testsuite/gas/riscv/march-fail-rv64i_v.l |  3 +++
 3 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 gas/testsuite/gas/riscv/march-fail-rv64i_v.d
 create mode 100644 gas/testsuite/gas/riscv/march-fail-rv64i_v.l

Comments

Jan Beulich Jan. 21, 2025, 3:48 p.m. UTC | #1 
On 21.01.2025 16:23, Jiawei wrote:
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -2123,7 +2123,15 @@ riscv_parse_check_conflicts (riscv_parse_subset_t *rps)
>  	(_("`xtheadvector' is conflict with the `v' extension"));
>        no_conflict = false;
>      }
> -
> +  /* We might use a multiplication to calculate the scalable vector length at
> +     runtime.  Therefore, require the M extension.  */
> +  if (riscv_lookup_subset (rps->subset_list, "v", &subset)
> +      && !riscv_lookup_subset (rps->subset_list, "m", &subset))
> +    {
> +      rps->error_handler
> +	(_("Currently the 'v' implementation requires the 'm' extension"));
> +      no_conflict = false;
> +    }

Who or what is "we" in the comment? The impression I'm getting is that
you talk of other people's / project's code, which we shouldn't put
constraints on. Otherwise could you please point me (and possibly
others) at the multiplication code injected by binutils behind the
user's back?

Jan
Christoph Müllner Jan. 21, 2025, 3:55 p.m. UTC | #2 
On Tue, Jan 21, 2025 at 4:23 PM Jiawei <jiawei@iscas.ac.cn> wrote:
>
> This patch add a new extension check when using `v` extension, it
> currently should also enable `m` extension to make sure vector length
> caculate correct.

How to trigger this incorrect calculation?
Is there an example?

>
> bfd/ChangeLog:
>
>         * elfxx-riscv.c (riscv_parse_check_conflicts): New check.
>
> gas/ChangeLog:
>
>         * testsuite/gas/riscv/march-fail-rv64i_v.d: New test.
>         * testsuite/gas/riscv/march-fail-rv64i_v.l: New test.
>
> ---
>  bfd/elfxx-riscv.c                            | 10 +++++++++-
>  gas/testsuite/gas/riscv/march-fail-rv64i_v.d |  4 ++++
>  gas/testsuite/gas/riscv/march-fail-rv64i_v.l |  3 +++
>  3 files changed, 16 insertions(+), 1 deletion(-)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-rv64i_v.d
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-rv64i_v.l
>
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index c9e4b03b17d..150838d3d24 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -2123,7 +2123,15 @@ riscv_parse_check_conflicts (riscv_parse_subset_t *rps)
>         (_("`xtheadvector' is conflict with the `v' extension"));
>        no_conflict = false;
>      }
> -
> +  /* We might use a multiplication to calculate the scalable vector length at
> +     runtime.  Therefore, require the M extension.  */
> +  if (riscv_lookup_subset (rps->subset_list, "v", &subset)
> +      && !riscv_lookup_subset (rps->subset_list, "m", &subset))
> +    {
> +      rps->error_handler
> +       (_("Currently the 'v' implementation requires the 'm' extension"));
> +      no_conflict = false;
> +    }
>    bool support_zve = false;
>    bool support_zvl = false;
>    riscv_subset_t *s = rps->subset_list->head;
> diff --git a/gas/testsuite/gas/riscv/march-fail-rv64i_v.d b/gas/testsuite/gas/riscv/march-fail-rv64i_v.d
> new file mode 100644
> index 00000000000..4d14bb4f9a9
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-rv64i_v.d
> @@ -0,0 +1,4 @@
> +#as: -march=rv64i_v
> +#source: empty.s
> +#error_output: march-fail-rv64i_v.l
> +
> diff --git a/gas/testsuite/gas/riscv/march-fail-rv64i_v.l b/gas/testsuite/gas/riscv/march-fail-rv64i_v.l
> new file mode 100644
> index 00000000000..5386a942ec5
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-rv64i_v.l
> @@ -0,0 +1,3 @@
> +Assembler messages:
> +Error: Currently the 'v' implementation requires the 'm' extension
> +
> --
> 2.43.0
>
Jiawei Jan. 22, 2025, 12:57 a.m. UTC | #3 
在 2025/1/21 23:48, Jan Beulich 写道:
> On 21.01.2025 16:23, Jiawei wrote:
>> --- a/bfd/elfxx-riscv.c
>> +++ b/bfd/elfxx-riscv.c
>> @@ -2123,7 +2123,15 @@ riscv_parse_check_conflicts (riscv_parse_subset_t *rps)
>>   	(_("`xtheadvector' is conflict with the `v' extension"));
>>         no_conflict = false;
>>       }
>> -
>> +  /* We might use a multiplication to calculate the scalable vector length at
>> +     runtime.  Therefore, require the M extension.  */
>> +  if (riscv_lookup_subset (rps->subset_list, "v", &subset)
>> +      && !riscv_lookup_subset (rps->subset_list, "m", &subset))
>> +    {
>> +      rps->error_handler
>> +	(_("Currently the 'v' implementation requires the 'm' extension"));
>> +      no_conflict = false;
>> +    }
> Who or what is "we" in the comment? The impression I'm getting is that
> you talk of other people's / project's code, which we shouldn't put
> constraints on. Otherwise could you please point me (and possibly
> others) at the multiplication code injected by binutils behind the
> user's back?
>
> Jan

Thanks for your review, "we" refers to some RVV developers in gcc.

Since I found currently the behaviour in gas is different with gcc part,

when I use '-march=rv64iv' in gcc part, it comes an error, see:


https://godbolt.org/z/W46fT1h6x

So I want to sync the behaviour on gcc and binutils part on vector check.

If I am mistaken here, please feel free to correct me.

BR,

Jiawei
Jiawei Jan. 22, 2025, 1:11 a.m. UTC | #4 
在 2025/1/21 23:55, Christoph Müllner 写道:
> On Tue, Jan 21, 2025 at 4:23 PM Jiawei <jiawei@iscas.ac.cn> wrote:
>> This patch add a new extension check when using `v` extension, it
>> currently should also enable `m` extension to make sure vector length
>> caculate correct.
> How to trigger this incorrect calculation?
> Is there an example?

I found this commit in gcc refer to this, please check it:

https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=e589ffb6d78881572ddea21df0d9b6c2641d574d;hp=4cbbce045681c234387d8d56376ea179dc869229


BR,

Jiawei

>> bfd/ChangeLog:
>>
>>          * elfxx-riscv.c (riscv_parse_check_conflicts): New check.
>>
>> gas/ChangeLog:
>>
>>          * testsuite/gas/riscv/march-fail-rv64i_v.d: New test.
>>          * testsuite/gas/riscv/march-fail-rv64i_v.l: New test.
>>
>> ---
>>   bfd/elfxx-riscv.c                            | 10 +++++++++-
>>   gas/testsuite/gas/riscv/march-fail-rv64i_v.d |  4 ++++
>>   gas/testsuite/gas/riscv/march-fail-rv64i_v.l |  3 +++
>>   3 files changed, 16 insertions(+), 1 deletion(-)
>>   create mode 100644 gas/testsuite/gas/riscv/march-fail-rv64i_v.d
>>   create mode 100644 gas/testsuite/gas/riscv/march-fail-rv64i_v.l
>>
>> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
>> index c9e4b03b17d..150838d3d24 100644
>> --- a/bfd/elfxx-riscv.c
>> +++ b/bfd/elfxx-riscv.c
>> @@ -2123,7 +2123,15 @@ riscv_parse_check_conflicts (riscv_parse_subset_t *rps)
>>          (_("`xtheadvector' is conflict with the `v' extension"));
>>         no_conflict = false;
>>       }
>> -
>> +  /* We might use a multiplication to calculate the scalable vector length at
>> +     runtime.  Therefore, require the M extension.  */
>> +  if (riscv_lookup_subset (rps->subset_list, "v", &subset)
>> +      && !riscv_lookup_subset (rps->subset_list, "m", &subset))
>> +    {
>> +      rps->error_handler
>> +       (_("Currently the 'v' implementation requires the 'm' extension"));
>> +      no_conflict = false;
>> +    }
>>     bool support_zve = false;
>>     bool support_zvl = false;
>>     riscv_subset_t *s = rps->subset_list->head;
>> diff --git a/gas/testsuite/gas/riscv/march-fail-rv64i_v.d b/gas/testsuite/gas/riscv/march-fail-rv64i_v.d
>> new file mode 100644
>> index 00000000000..4d14bb4f9a9
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/march-fail-rv64i_v.d
>> @@ -0,0 +1,4 @@
>> +#as: -march=rv64i_v
>> +#source: empty.s
>> +#error_output: march-fail-rv64i_v.l
>> +
>> diff --git a/gas/testsuite/gas/riscv/march-fail-rv64i_v.l b/gas/testsuite/gas/riscv/march-fail-rv64i_v.l
>> new file mode 100644
>> index 00000000000..5386a942ec5
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/march-fail-rv64i_v.l
>> @@ -0,0 +1,3 @@
>> +Assembler messages:
>> +Error: Currently the 'v' implementation requires the 'm' extension
>> +
>> --
>> 2.43.0
>>
Christoph Müllner Jan. 22, 2025, 1:18 a.m. UTC | #5 
On Wed, Jan 22, 2025 at 2:11 AM Jiawei <jiawei@iscas.ac.cn> wrote:
>
>
> 在 2025/1/21 23:55, Christoph Müllner 写道:
> > On Tue, Jan 21, 2025 at 4:23 PM Jiawei <jiawei@iscas.ac.cn> wrote:
> >> This patch add a new extension check when using `v` extension, it
> >> currently should also enable `m` extension to make sure vector length
> >> caculate correct.
> > How to trigger this incorrect calculation?
> > Is there an example?
>
> I found this commit in gcc refer to this, please check it:
>
> https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=e589ffb6d78881572ddea21df0d9b6c2641d574d;hp=4cbbce045681c234387d8d56376ea179dc869229

This means that GCC's RISC-V vectorizer backend depends on M.
However, the V extension does not.
And it is possible to write valid assembly files that include V
instructions but no M instructions.

So, if GCC needs M, then it is reasonable that GCC requests M to be enabled.
However, this is independent of Binutils.

>
>
> BR,
>
> Jiawei
>
> >> bfd/ChangeLog:
> >>
> >>          * elfxx-riscv.c (riscv_parse_check_conflicts): New check.
> >>
> >> gas/ChangeLog:
> >>
> >>          * testsuite/gas/riscv/march-fail-rv64i_v.d: New test.
> >>          * testsuite/gas/riscv/march-fail-rv64i_v.l: New test.
> >>
> >> ---
> >>   bfd/elfxx-riscv.c                            | 10 +++++++++-
> >>   gas/testsuite/gas/riscv/march-fail-rv64i_v.d |  4 ++++
> >>   gas/testsuite/gas/riscv/march-fail-rv64i_v.l |  3 +++
> >>   3 files changed, 16 insertions(+), 1 deletion(-)
> >>   create mode 100644 gas/testsuite/gas/riscv/march-fail-rv64i_v.d
> >>   create mode 100644 gas/testsuite/gas/riscv/march-fail-rv64i_v.l
> >>
> >> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> >> index c9e4b03b17d..150838d3d24 100644
> >> --- a/bfd/elfxx-riscv.c
> >> +++ b/bfd/elfxx-riscv.c
> >> @@ -2123,7 +2123,15 @@ riscv_parse_check_conflicts (riscv_parse_subset_t *rps)
> >>          (_("`xtheadvector' is conflict with the `v' extension"));
> >>         no_conflict = false;
> >>       }
> >> -
> >> +  /* We might use a multiplication to calculate the scalable vector length at
> >> +     runtime.  Therefore, require the M extension.  */
> >> +  if (riscv_lookup_subset (rps->subset_list, "v", &subset)
> >> +      && !riscv_lookup_subset (rps->subset_list, "m", &subset))
> >> +    {
> >> +      rps->error_handler
> >> +       (_("Currently the 'v' implementation requires the 'm' extension"));
> >> +      no_conflict = false;
> >> +    }
> >>     bool support_zve = false;
> >>     bool support_zvl = false;
> >>     riscv_subset_t *s = rps->subset_list->head;
> >> diff --git a/gas/testsuite/gas/riscv/march-fail-rv64i_v.d b/gas/testsuite/gas/riscv/march-fail-rv64i_v.d
> >> new file mode 100644
> >> index 00000000000..4d14bb4f9a9
> >> --- /dev/null
> >> +++ b/gas/testsuite/gas/riscv/march-fail-rv64i_v.d
> >> @@ -0,0 +1,4 @@
> >> +#as: -march=rv64i_v
> >> +#source: empty.s
> >> +#error_output: march-fail-rv64i_v.l
> >> +
> >> diff --git a/gas/testsuite/gas/riscv/march-fail-rv64i_v.l b/gas/testsuite/gas/riscv/march-fail-rv64i_v.l
> >> new file mode 100644
> >> index 00000000000..5386a942ec5
> >> --- /dev/null
> >> +++ b/gas/testsuite/gas/riscv/march-fail-rv64i_v.l
> >> @@ -0,0 +1,3 @@
> >> +Assembler messages:
> >> +Error: Currently the 'v' implementation requires the 'm' extension
> >> +
> >> --
> >> 2.43.0
> >>
>
Jiawei Jan. 22, 2025, 1:25 a.m. UTC | #6 
在 2025/1/22 9:18, Christoph Müllner 写道:
> On Wed, Jan 22, 2025 at 2:11 AM Jiawei <jiawei@iscas.ac.cn> wrote:
>>
>> 在 2025/1/21 23:55, Christoph Müllner 写道:
>>> On Tue, Jan 21, 2025 at 4:23 PM Jiawei <jiawei@iscas.ac.cn> wrote:
>>>> This patch add a new extension check when using `v` extension, it
>>>> currently should also enable `m` extension to make sure vector length
>>>> caculate correct.
>>> How to trigger this incorrect calculation?
>>> Is there an example?
>> I found this commit in gcc refer to this, please check it:
>>
>> https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=e589ffb6d78881572ddea21df0d9b6c2641d574d;hp=4cbbce045681c234387d8d56376ea179dc869229
> This means that GCC's RISC-V vectorizer backend depends on M.
> However, the V extension does not.
> And it is possible to write valid assembly files that include V
> instructions but no M instructions.
>
> So, if GCC needs M, then it is reasonable that GCC requests M to be enabled.
> However, this is independent of Binutils.
Thank you for your explanation, it corrected my misunderstanding.
>>
>> BR,
>>
>> Jiawei
>>
>>>> bfd/ChangeLog:
>>>>
>>>>           * elfxx-riscv.c (riscv_parse_check_conflicts): New check.
>>>>
>>>> gas/ChangeLog:
>>>>
>>>>           * testsuite/gas/riscv/march-fail-rv64i_v.d: New test.
>>>>           * testsuite/gas/riscv/march-fail-rv64i_v.l: New test.
>>>>
>>>> ---
>>>>    bfd/elfxx-riscv.c                            | 10 +++++++++-
>>>>    gas/testsuite/gas/riscv/march-fail-rv64i_v.d |  4 ++++
>>>>    gas/testsuite/gas/riscv/march-fail-rv64i_v.l |  3 +++
>>>>    3 files changed, 16 insertions(+), 1 deletion(-)
>>>>    create mode 100644 gas/testsuite/gas/riscv/march-fail-rv64i_v.d
>>>>    create mode 100644 gas/testsuite/gas/riscv/march-fail-rv64i_v.l
>>>>
>>>> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
>>>> index c9e4b03b17d..150838d3d24 100644
>>>> --- a/bfd/elfxx-riscv.c
>>>> +++ b/bfd/elfxx-riscv.c
>>>> @@ -2123,7 +2123,15 @@ riscv_parse_check_conflicts (riscv_parse_subset_t *rps)
>>>>           (_("`xtheadvector' is conflict with the `v' extension"));
>>>>          no_conflict = false;
>>>>        }
>>>> -
>>>> +  /* We might use a multiplication to calculate the scalable vector length at
>>>> +     runtime.  Therefore, require the M extension.  */
>>>> +  if (riscv_lookup_subset (rps->subset_list, "v", &subset)
>>>> +      && !riscv_lookup_subset (rps->subset_list, "m", &subset))
>>>> +    {
>>>> +      rps->error_handler
>>>> +       (_("Currently the 'v' implementation requires the 'm' extension"));
>>>> +      no_conflict = false;
>>>> +    }
>>>>      bool support_zve = false;
>>>>      bool support_zvl = false;
>>>>      riscv_subset_t *s = rps->subset_list->head;
>>>> diff --git a/gas/testsuite/gas/riscv/march-fail-rv64i_v.d b/gas/testsuite/gas/riscv/march-fail-rv64i_v.d
>>>> new file mode 100644
>>>> index 00000000000..4d14bb4f9a9
>>>> --- /dev/null
>>>> +++ b/gas/testsuite/gas/riscv/march-fail-rv64i_v.d
>>>> @@ -0,0 +1,4 @@
>>>> +#as: -march=rv64i_v
>>>> +#source: empty.s
>>>> +#error_output: march-fail-rv64i_v.l
>>>> +
>>>> diff --git a/gas/testsuite/gas/riscv/march-fail-rv64i_v.l b/gas/testsuite/gas/riscv/march-fail-rv64i_v.l
>>>> new file mode 100644
>>>> index 00000000000..5386a942ec5
>>>> --- /dev/null
>>>> +++ b/gas/testsuite/gas/riscv/march-fail-rv64i_v.l
>>>> @@ -0,0 +1,3 @@
>>>> +Assembler messages:
>>>> +Error: Currently the 'v' implementation requires the 'm' extension
>>>> +
>>>> --
>>>> 2.43.0
>>>>
diff mbox series

Patch

diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
index c9e4b03b17d..150838d3d24 100644
--- a/bfd/elfxx-riscv.c
+++ b/bfd/elfxx-riscv.c
@@ -2123,7 +2123,15 @@  riscv_parse_check_conflicts (riscv_parse_subset_t *rps)
 	(_("`xtheadvector' is conflict with the `v' extension"));
       no_conflict = false;
     }
-
+  /* We might use a multiplication to calculate the scalable vector length at
+     runtime.  Therefore, require the M extension.  */
+  if (riscv_lookup_subset (rps->subset_list, "v", &subset)
+      && !riscv_lookup_subset (rps->subset_list, "m", &subset))
+    {
+      rps->error_handler
+	(_("Currently the 'v' implementation requires the 'm' extension"));
+      no_conflict = false;
+    }
   bool support_zve = false;
   bool support_zvl = false;
   riscv_subset_t *s = rps->subset_list->head;
diff --git a/gas/testsuite/gas/riscv/march-fail-rv64i_v.d b/gas/testsuite/gas/riscv/march-fail-rv64i_v.d
new file mode 100644
index 00000000000..4d14bb4f9a9
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-rv64i_v.d
@@ -0,0 +1,4 @@ 
+#as: -march=rv64i_v
+#source: empty.s
+#error_output: march-fail-rv64i_v.l
+
diff --git a/gas/testsuite/gas/riscv/march-fail-rv64i_v.l b/gas/testsuite/gas/riscv/march-fail-rv64i_v.l
new file mode 100644
index 00000000000..5386a942ec5
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-rv64i_v.l
@@ -0,0 +1,3 @@ 
+Assembler messages:
+Error: Currently the 'v' implementation requires the 'm' extension
+