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

[Kito Cheng <kito.cheng@gmail.com>]

-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
> >