[PATCH] RISC-V: Default to tuning for the thead-c906

[PATCH] RISC-V: Default to tuning for the thead-c906

[Palmer Dabbelt]

The C906 is by far the most widely available RISC-V processor, so let's
default to tuning for it.

gcc/ChangeLog

	* config/riscv/riscv.h (RISCV_TUNE_STRING_DEFAULT): Change to
	thead-c906.
	* doc/invoke.texi (RISC-V -mtune): Change the default to
	thead-c906.

---

This has come up a handful of times, most recently during the Cauldron.
It seems like a grey area to me: we're changing the behavior of some
command-line arguments (ie, everything that doesn't specify -mtune), but
we sort of change that anyway as the tuning parameters change between
releases.

I'm not really seeing much of a precedent from the other ports.  It
looks like aarch64 sort of changed the default in 02fdbd5beb0
("[AArch64] [-mtune cleanup 2/5] Tune for Cortex-A53 by default.") but I
think at that point -mtune=generic and -mtune=cortex-a53 were equivalent
so I'm not sure that counts.  I can't quite sort out if the default x86
tuning has ever changed, but the tuning parameters have changed.  I
don't see any way around having the tuning parameters change as they're
pretty tightly coupled to the GCC internals, but changing to a different
tuning target is a bit bigger of a change.

We also have a bit of a special case here: -mtune is in theory only a
performance issue, but this change will emit a lot more misaligned
accesses and we've seen those trigger bugs in the trap handlers before.
Those bugs are elsewhere so it's sort of not a GCC problem, but I'm sure
there's still users out there with broken firmware and this may cause
visible fallout.  We can just tell those users their systems were always
broken, but that's never a fun way to do things.

I figured the easiest way to talk about this would be to just send the
patch, but I definitely don't plan on committing it without some
discussion.
---
 gcc/config/riscv/riscv.h | 2 +-
 gcc/doc/invoke.texi      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
index 363113c6511..1d9379fa5ee 100644
--- a/gcc/config/riscv/riscv.h
+++ b/gcc/config/riscv/riscv.h
@@ -40,7 +40,7 @@ along with GCC; see the file COPYING3.  If not see
 #endif
 
 #ifndef RISCV_TUNE_STRING_DEFAULT
-#define RISCV_TUNE_STRING_DEFAULT "rocket"
+#define RISCV_TUNE_STRING_DEFAULT "thead-c906"
 #endif
 
 extern const char *riscv_expand_arch (int argc, const char **argv);
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index e0c2c57c9b2..2a9ea3455f6 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -28529,7 +28529,7 @@ particular CPU name.  Permissible values for this option are: @samp{rocket},
 @samp{thead-c906}, @samp{size}, and all valid options for @option{-mcpu=}.
 
 When @option{-mtune=} is not specified, use the setting from @option{-mcpu},
-the default is @samp{rocket} if both are not specified.
+the default is @samp{thead-c906} if both are not specified.
 
 The @samp{size} choice is not intended for use by end-users.  This is used
 when @option{-Os} is specified.  It overrides the instruction cost info
-- 
2.34.1

Re: [PATCH] RISC-V: Default to tuning for the thead-c906

[Andrew Pinski]

On Tue, Oct 4, 2022 at 8:55 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> The C906 is by far the most widely available RISC-V processor, so let's
> default to tuning for it.
>
> gcc/ChangeLog
>
>         * config/riscv/riscv.h (RISCV_TUNE_STRING_DEFAULT): Change to
>         thead-c906.
>         * doc/invoke.texi (RISC-V -mtune): Change the default to
>         thead-c906.
>
> ---

I am ok with this as --with-tune and --with-arch works as ways of
changing the default still.

Thanks,
Andrew

>
> This has come up a handful of times, most recently during the Cauldron.
> It seems like a grey area to me: we're changing the behavior of some
> command-line arguments (ie, everything that doesn't specify -mtune), but
> we sort of change that anyway as the tuning parameters change between
> releases.
>
> I'm not really seeing much of a precedent from the other ports.  It
> looks like aarch64 sort of changed the default in 02fdbd5beb0
> ("[AArch64] [-mtune cleanup 2/5] Tune for Cortex-A53 by default.") but I
> think at that point -mtune=generic and -mtune=cortex-a53 were equivalent
> so I'm not sure that counts.  I can't quite sort out if the default x86
> tuning has ever changed, but the tuning parameters have changed.  I
> don't see any way around having the tuning parameters change as they're
> pretty tightly coupled to the GCC internals, but changing to a different
> tuning target is a bit bigger of a change.
>
> We also have a bit of a special case here: -mtune is in theory only a
> performance issue, but this change will emit a lot more misaligned
> accesses and we've seen those trigger bugs in the trap handlers before.
> Those bugs are elsewhere so it's sort of not a GCC problem, but I'm sure
> there's still users out there with broken firmware and this may cause
> visible fallout.  We can just tell those users their systems were always
> broken, but that's never a fun way to do things.
>
> I figured the easiest way to talk about this would be to just send the
> patch, but I definitely don't plan on committing it without some
> discussion.
> ---
>  gcc/config/riscv/riscv.h | 2 +-
>  gcc/doc/invoke.texi      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
> index 363113c6511..1d9379fa5ee 100644
> --- a/gcc/config/riscv/riscv.h
> +++ b/gcc/config/riscv/riscv.h
> @@ -40,7 +40,7 @@ along with GCC; see the file COPYING3.  If not see
>  #endif
>
>  #ifndef RISCV_TUNE_STRING_DEFAULT
> -#define RISCV_TUNE_STRING_DEFAULT "rocket"
> +#define RISCV_TUNE_STRING_DEFAULT "thead-c906"
>  #endif
>
>  extern const char *riscv_expand_arch (int argc, const char **argv);
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index e0c2c57c9b2..2a9ea3455f6 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -28529,7 +28529,7 @@ particular CPU name.  Permissible values for this option are: @samp{rocket},
>  @samp{thead-c906}, @samp{size}, and all valid options for @option{-mcpu=}.
>
>  When @option{-mtune=} is not specified, use the setting from @option{-mcpu},
> -the default is @samp{rocket} if both are not specified.
> +the default is @samp{thead-c906} if both are not specified.
>
>  The @samp{size} choice is not intended for use by end-users.  This is used
>  when @option{-Os} is specified.  It overrides the instruction cost info
> --
> 2.34.1
>

Re: [PATCH] RISC-V: Default to tuning for the thead-c906

[Kito Cheng]

-1 for this, default enable fast unaligned access could cause many
problems, and lots of RISC-V cores
don't support HW unaligned access (Rocket-base RISC-V core, most
SiFive core, and most Andes core IIRC),
change this to default means package from RISC-V linux distro might
contain unaligned access by default.

The default one should be the safest option, in case people really
want that, they could use --with-tune and
--with-arch to change that.


On Wed, Oct 5, 2022 at 1:57 PM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Tue, Oct 4, 2022 at 8:55 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
> >
> > The C906 is by far the most widely available RISC-V processor, so let's
> > default to tuning for it.
> >
> > gcc/ChangeLog
> >
> >         * config/riscv/riscv.h (RISCV_TUNE_STRING_DEFAULT): Change to
> >         thead-c906.
> >         * doc/invoke.texi (RISC-V -mtune): Change the default to
> >         thead-c906.
> >
> > ---
>
> I am ok with this as --with-tune and --with-arch works as ways of
> changing the default still.
>
> Thanks,
> Andrew
>
> >
> > This has come up a handful of times, most recently during the Cauldron.
> > It seems like a grey area to me: we're changing the behavior of some
> > command-line arguments (ie, everything that doesn't specify -mtune), but
> > we sort of change that anyway as the tuning parameters change between
> > releases.
> >
> > I'm not really seeing much of a precedent from the other ports.  It
> > looks like aarch64 sort of changed the default in 02fdbd5beb0
> > ("[AArch64] [-mtune cleanup 2/5] Tune for Cortex-A53 by default.") but I
> > think at that point -mtune=generic and -mtune=cortex-a53 were equivalent
> > so I'm not sure that counts.  I can't quite sort out if the default x86
> > tuning has ever changed, but the tuning parameters have changed.  I
> > don't see any way around having the tuning parameters change as they're
> > pretty tightly coupled to the GCC internals, but changing to a different
> > tuning target is a bit bigger of a change.
> >
> > We also have a bit of a special case here: -mtune is in theory only a
> > performance issue, but this change will emit a lot more misaligned
> > accesses and we've seen those trigger bugs in the trap handlers before.
> > Those bugs are elsewhere so it's sort of not a GCC problem, but I'm sure
> > there's still users out there with broken firmware and this may cause
> > visible fallout.  We can just tell those users their systems were always
> > broken, but that's never a fun way to do things.
> >
> > I figured the easiest way to talk about this would be to just send the
> > patch, but I definitely don't plan on committing it without some
> > discussion.
> > ---
> >  gcc/config/riscv/riscv.h | 2 +-
> >  gcc/doc/invoke.texi      | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
> > index 363113c6511..1d9379fa5ee 100644
> > --- a/gcc/config/riscv/riscv.h
> > +++ b/gcc/config/riscv/riscv.h
> > @@ -40,7 +40,7 @@ along with GCC; see the file COPYING3.  If not see
> >  #endif
> >
> >  #ifndef RISCV_TUNE_STRING_DEFAULT
> > -#define RISCV_TUNE_STRING_DEFAULT "rocket"
> > +#define RISCV_TUNE_STRING_DEFAULT "thead-c906"
> >  #endif
> >
> >  extern const char *riscv_expand_arch (int argc, const char **argv);
> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index e0c2c57c9b2..2a9ea3455f6 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -28529,7 +28529,7 @@ particular CPU name.  Permissible values for this option are: @samp{rocket},
> >  @samp{thead-c906}, @samp{size}, and all valid options for @option{-mcpu=}.
> >
> >  When @option{-mtune=} is not specified, use the setting from @option{-mcpu},
> > -the default is @samp{rocket} if both are not specified.
> > +the default is @samp{thead-c906} if both are not specified.
> >
> >  The @samp{size} choice is not intended for use by end-users.  This is used
> >  when @option{-Os} is specified.  It overrides the instruction cost info
> > --
> > 2.34.1
> >

Re: [PATCH] RISC-V: Default to tuning for the thead-c906

[Andrew Waterman]

I agree with Kito; I don't support merging this patch.  My reasoning is twofold:

- The default settings should be fairly neutral, avoiding codegen that
severely disadvantages some targets.  Misaligned memory accesses are
certainly a problematic case in that respect.  (And it's highly
asymmetric: the perf upside for implementations that do support this
feature is much lower than the perf downside for implementations that
don't.)  I haven't reviewed the rest of the C906 tuning decisions, but
I'd raise the same concern if any of them are similarly non-neutral.

- Both the RISC-V ISA spec and the more recent RVA22 profile spec
explicitly caution about the perf impacts of such a tuning decision.
The RVA22 profile spec goes further to explicitly state that standard
software distributions should not do what this patch does.
https://github.com/riscv/riscv-profiles/commit/b09dc934e8cb8c1dfb7ddead8bdf509408e94609#diff-01b3848d5e44a205fafe2a7b93dbf353138763b76110bb833b7eeee1ac4afe5bR593-R595


Andrew


On Wed, Oct 5, 2022 at 6:59 PM Kito Cheng via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> -1 for this, default enable fast unaligned access could cause many
> problems, and lots of RISC-V cores
> don't support HW unaligned access (Rocket-base RISC-V core, most
> SiFive core, and most Andes core IIRC),
> change this to default means package from RISC-V linux distro might
> contain unaligned access by default.
>
> The default one should be the safest option, in case people really
> want that, they could use --with-tune and
> --with-arch to change that.
>
>
> On Wed, Oct 5, 2022 at 1:57 PM Andrew Pinski via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > On Tue, Oct 4, 2022 at 8:55 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
> > >
> > > The C906 is by far the most widely available RISC-V processor, so let's
> > > default to tuning for it.
> > >
> > > gcc/ChangeLog
> > >
> > >         * config/riscv/riscv.h (RISCV_TUNE_STRING_DEFAULT): Change to
> > >         thead-c906.
> > >         * doc/invoke.texi (RISC-V -mtune): Change the default to
> > >         thead-c906.
> > >
> > > ---
> >
> > I am ok with this as --with-tune and --with-arch works as ways of
> > changing the default still.
> >
> > Thanks,
> > Andrew
> >
> > >
> > > This has come up a handful of times, most recently during the Cauldron.
> > > It seems like a grey area to me: we're changing the behavior of some
> > > command-line arguments (ie, everything that doesn't specify -mtune), but
> > > we sort of change that anyway as the tuning parameters change between
> > > releases.
> > >
> > > I'm not really seeing much of a precedent from the other ports.  It
> > > looks like aarch64 sort of changed the default in 02fdbd5beb0
> > > ("[AArch64] [-mtune cleanup 2/5] Tune for Cortex-A53 by default.") but I
> > > think at that point -mtune=generic and -mtune=cortex-a53 were equivalent
> > > so I'm not sure that counts.  I can't quite sort out if the default x86
> > > tuning has ever changed, but the tuning parameters have changed.  I
> > > don't see any way around having the tuning parameters change as they're
> > > pretty tightly coupled to the GCC internals, but changing to a different
> > > tuning target is a bit bigger of a change.
> > >
> > > We also have a bit of a special case here: -mtune is in theory only a
> > > performance issue, but this change will emit a lot more misaligned
> > > accesses and we've seen those trigger bugs in the trap handlers before.
> > > Those bugs are elsewhere so it's sort of not a GCC problem, but I'm sure
> > > there's still users out there with broken firmware and this may cause
> > > visible fallout.  We can just tell those users their systems were always
> > > broken, but that's never a fun way to do things.
> > >
> > > I figured the easiest way to talk about this would be to just send the
> > > patch, but I definitely don't plan on committing it without some
> > > discussion.
> > > ---
> > >  gcc/config/riscv/riscv.h | 2 +-
> > >  gcc/doc/invoke.texi      | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
> > > index 363113c6511..1d9379fa5ee 100644
> > > --- a/gcc/config/riscv/riscv.h
> > > +++ b/gcc/config/riscv/riscv.h
> > > @@ -40,7 +40,7 @@ along with GCC; see the file COPYING3.  If not see
> > >  #endif
> > >
> > >  #ifndef RISCV_TUNE_STRING_DEFAULT
> > > -#define RISCV_TUNE_STRING_DEFAULT "rocket"
> > > +#define RISCV_TUNE_STRING_DEFAULT "thead-c906"
> > >  #endif
> > >
> > >  extern const char *riscv_expand_arch (int argc, const char **argv);
> > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > > index e0c2c57c9b2..2a9ea3455f6 100644
> > > --- a/gcc/doc/invoke.texi
> > > +++ b/gcc/doc/invoke.texi
> > > @@ -28529,7 +28529,7 @@ particular CPU name.  Permissible values for this option are: @samp{rocket},
> > >  @samp{thead-c906}, @samp{size}, and all valid options for @option{-mcpu=}.
> > >
> > >  When @option{-mtune=} is not specified, use the setting from @option{-mcpu},
> > > -the default is @samp{rocket} if both are not specified.
> > > +the default is @samp{thead-c906} if both are not specified.
> > >
> > >  The @samp{size} choice is not intended for use by end-users.  This is used
> > >  when @option{-Os} is specified.  It overrides the instruction cost info
> > > --
> > > 2.34.1
> > >