[PATCH] RISC-V: Allow ISA subsets to be disabled

[PATCH] RISC-V: Allow ISA subsets to be disabled

[Palmer Dabbelt]

Without this patch, passing "-march=rv64ic -march=rv64i" results in
you getting a "RV64IC" toolchain, which isn't expected.

gas/ChangeLog:

2017-03-30  Palmer Dabbelt  <palmer@dabbelt.com>

       * config/tc-riscv.c (riscv_clear_subsets): New function.
       (riscv_add_subset): Call riscv_clear_subsets and riscv_set_rvc to
       clear RVC when it's been previously set.
---
 gas/ChangeLog         |  6 ++++++
 gas/config/tc-riscv.c | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/gas/ChangeLog b/gas/ChangeLog
index a5c1d08..6f419f7 100644
--- a/gas/ChangeLog
+++ b/gas/ChangeLog
@@ -1,3 +1,9 @@
+2017-03-30  Palmer Dabbelt  <palmer@dabbelt.com>
+
+	* config/tc-riscv.c (riscv_clear_subsets): New function.
+	(riscv_add_subset): Call riscv_clear_subsets and riscv_set_rvc to
+	clear RVC when it's been previously set.
+
 2017-03-30  Pip Cet  <pipcet@gmail.com>
 
 	* config/tc-wasm32.h: New file: Add WebAssembly assembler target.
diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 68b28f7..2830ba1 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -121,6 +121,18 @@ riscv_subset_supports (const char *feature)
 }
 
 static void
+riscv_clear_subsets (void)
+{
+  while (riscv_subsets != NULL)
+    {
+      struct riscv_subset *next = riscv_subsets->next;
+      free (riscv_subsets->name);
+      free (riscv_subsets);
+      riscv_subsets = next;
+    }
+}
+
+static void
 riscv_add_subset (const char *subset)
 {
   struct riscv_subset *s = xmalloc (sizeof *s);
@@ -139,6 +151,8 @@ riscv_set_arch (const char *s)
   const char *extension = NULL;
   const char *p = s;
 
+  riscv_clear_subsets();
+
   if (strncmp (p, "rv32", 4) == 0)
     {
       xlen = 32;
@@ -1808,6 +1822,7 @@ riscv_after_parse_args (void)
     riscv_set_arch (xlen == 64 ? "rv64g" : "rv32g");
 
   /* Add the RVC extension, regardless of -march, to support .option rvc.  */
+  riscv_set_rvc (FALSE);
   if (riscv_subset_supports ("c"))
     riscv_set_rvc (TRUE);
   else
-- 
2.10.2

Re: [PATCH] RISC-V: Allow ISA subsets to be disabled

[Alex Bradbury]

On 30 March 2017 at 20:45, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> Without this patch, passing "-march=rv64ic -march=rv64i" results in
> you getting a "RV64IC" toolchain, which isn't expected.
>
> gas/ChangeLog:
>
> 2017-03-30  Palmer Dabbelt  <palmer@dabbelt.com>
>
>        * config/tc-riscv.c (riscv_clear_subsets): New function.
>        (riscv_add_subset): Call riscv_clear_subsets and riscv_set_rvc to
>        clear RVC when it's been previously set.

For what it's worth, the current behaviour (i.e. the behaviour before
this patch) seems to be in line with other architectures. e.g. if I do
`gcc -march=native -march=generic` then the latter won't override the
former - at least as evidenced by `gcc -march=native -march=generic -E
-v - </dev/null`.

Best,

Alex

Re: [PATCH] RISC-V: Allow ISA subsets to be disabled

[Alex Bradbury]

On 31 March 2017 at 09:15, Alex Bradbury <asb@asbradbury.org> wrote:
> On 30 March 2017 at 20:45, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> Without this patch, passing "-march=rv64ic -march=rv64i" results in
>> you getting a "RV64IC" toolchain, which isn't expected.
>>
>> gas/ChangeLog:
>>
>> 2017-03-30  Palmer Dabbelt  <palmer@dabbelt.com>
>>
>>        * config/tc-riscv.c (riscv_clear_subsets): New function.
>>        (riscv_add_subset): Call riscv_clear_subsets and riscv_set_rvc to
>>        clear RVC when it's been previously set.
>
> For what it's worth, the current behaviour (i.e. the behaviour before
> this patch) seems to be in line with other architectures. e.g. if I do
> `gcc -march=native -march=generic` then the latter won't override the
> former - at least as evidenced by `gcc -march=native -march=generic -E
> -v - </dev/null`.

My apologies, over-riding behaviour _is_ standard for as which is
obviously the relevant comparison.

$ cat foo.s
foo:
    vmulsd    %xmm2, %xmm1, %xmm1
    vaddsd    %xmm0, %xmm1, %xmm0
    ret
$ as -march=corei7+avx -march=generic64 foo.s
foo.s: Assembler messages:
foo.s:2: Error: `vmulsd' is not supported on `generic64'
foo.s:3: Error: `vaddsd' is not supported on `generic64'

It's a shame gcc and as are inconsistent in their argument handling in
this way, at least on x86-64. Palmer: are you intending to make a
matching change to -march for gcc, or to stick with the non-overriding
behaviour -march has on x86 (and other platforms?).

Best,

Alex

Re: [PATCH] RISC-V: Allow ISA subsets to be disabled

[Nick Clifton]

Hi Palmer,

> 2017-03-30  Palmer Dabbelt  <palmer@dabbelt.com>
> 
>        * config/tc-riscv.c (riscv_clear_subsets): New function.
>        (riscv_add_subset): Call riscv_clear_subsets and riscv_set_rvc to
>        clear RVC when it's been previously set.

Approved - please apply.

Cheers
  Nick

Re: [PATCH] RISC-V: Allow ISA subsets to be disabled

[Nick Clifton]

Hi Alex,

> It's a shame gcc and as are inconsistent in their argument handling in
> this way, at least on x86-64.

Agreed.

I had always thought that a later option overriding an earlier option was
standard behaviour, but when I checked the gcc documentation I could not
find any specific requirement for this.  I was quite surprised.  Personally
I think that gcc is wrong.  To my mind it is intuitive that a later option
would override an earlier one.  If it does not then the tool concerned, at
the very least, ought to issue a warning message that the later option is
being ignored.

So, in summary - I think that this is a gcc bug, not a binutils bug.

Cheers
  Nick
 

Re: [PATCH] RISC-V: Allow ISA subsets to be disabled

[Palmer Dabbelt]

On Fri, 31 Mar 2017 02:22:53 PDT (-0700), Nick Clifton wrote:
> Hi Alex,
>
>> It's a shame gcc and as are inconsistent in their argument handling in
>> this way, at least on x86-64.
>
> Agreed.
>
> I had always thought that a later option overriding an earlier option was
> standard behaviour, but when I checked the gcc documentation I could not
> find any specific requirement for this.  I was quite surprised.  Personally
> I think that gcc is wrong.  To my mind it is intuitive that a later option
> would override an earlier one.  If it does not then the tool concerned, at
> the very least, ought to issue a warning message that the later option is
> being ignored.
>
> So, in summary - I think that this is a gcc bug, not a binutils bug.

I agree (though I guess that's implicit from having posted the patch :)).
We're trying really hard to make sure RISC-V has a sane set of machine-specific
options that are compatible everywhere (binutils, GCC, and LLVM) so we don't
get into this sort of mess in the future.

Committed.

[BUILDROBOT] RISC-V: error: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type (was: [PATCH] RISC-V: Allow ISA subsets to be disabled)

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

Hi Palmer,

On Thu, 2017-03-30 12:45:42 -0700, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> Without this patch, passing "-march=rv64ic -march=rv64i" results in
> you getting a "RV64IC" toolchain, which isn't expected.
> 
> gas/ChangeLog:
> 
> 2017-03-30  Palmer Dabbelt  <palmer@dabbelt.com>
> 
>        * config/tc-riscv.c (riscv_clear_subsets): New function.
>        (riscv_add_subset): Call riscv_clear_subsets and riscv_set_rvc to
>        clear RVC when it's been previously set.

Build robot catched a new warning in the build found at
http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=700765:

gcc -DHAVE_CONFIG_H -I. -I/home/jbglaw/repos/binutils_gdb/gas  -I. -I/home/jbglaw/repos/binutils_gdb/gas -I../bfd -I/home/jbglaw/repos/binutils_gdb/gas/config -I/home/jbglaw/repos/binutils_gdb/gas/../include -I/home/jbglaw/repos/binutils_gdb/gas/.. -I/home/jbglaw/repos/binutils_gdb/gas/../bfd -DLOCALEDIR="\"/home/jbglaw/build/riscv32-unknown-linux-gnu/_install_/share/locale\""  -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -Werror -Wwrite-strings -I/home/jbglaw/repos/binutils_gdb/gas/../zlib -g -O2 -MT tc-riscv.o -MD -MP -MF .deps/tc-riscv.Tpo -c -o tc-riscv.o `test -f 'config/tc-riscv.c' || echo '/home/jbglaw/repos/binutils_gdb/gas/'`config/tc-riscv.c
/home/jbglaw/repos/binutils_gdb/gas/config/tc-riscv.c: In function ‘riscv_clear_subsets’:
/home/jbglaw/repos/binutils_gdb/gas/config/tc-riscv.c:129:13: error: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [-Werror]
       free (riscv_subsets->name);
             ^
In file included from /home/jbglaw/repos/binutils_gdb/gas/as.h:58:0,
                 from /home/jbglaw/repos/binutils_gdb/gas/config/tc-riscv.c:23:
/usr/include/stdlib.h:483:13: note: expected ‘void *’ but argument is of type ‘const char *’
 extern void free (void *__ptr) __THROW;
             ^
cc1: all warnings being treated as errors
Makefile:1651: recipe for target 'tc-riscv.o' failed


MfG, JBG

-- 
      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481
  Signature of:                        Lauf nicht vor Deinem Glück davon:
  the second  :                             Es könnte hinter Dir stehen!

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]