* [PATCH] RISC-V: Default to tuning for the thead-c906 @ 2022-10-05 3:54 Palmer Dabbelt 2022-10-05 5:56 ` Andrew Pinski 0 siblings, 1 reply; 4+ messages in thread From: Palmer Dabbelt @ 2022-10-05 3:54 UTC (permalink / raw) To: gcc-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] RISC-V: Default to tuning for the thead-c906 2022-10-05 3:54 [PATCH] RISC-V: Default to tuning for the thead-c906 Palmer Dabbelt @ 2022-10-05 5:56 ` Andrew Pinski 2022-10-06 1:58 ` Kito Cheng 0 siblings, 1 reply; 4+ messages in thread From: Andrew Pinski @ 2022-10-05 5:56 UTC (permalink / raw) To: Palmer Dabbelt; +Cc: gcc-patches 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] RISC-V: Default to tuning for the thead-c906 2022-10-05 5:56 ` Andrew Pinski @ 2022-10-06 1:58 ` Kito Cheng 2022-10-06 2:45 ` Andrew Waterman 0 siblings, 1 reply; 4+ messages in thread From: Kito Cheng @ 2022-10-06 1:58 UTC (permalink / raw) To: Andrew Pinski; +Cc: Palmer Dabbelt, gcc-patches -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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] RISC-V: Default to tuning for the thead-c906 2022-10-06 1:58 ` Kito Cheng @ 2022-10-06 2:45 ` Andrew Waterman 0 siblings, 0 replies; 4+ messages in thread From: Andrew Waterman @ 2022-10-06 2:45 UTC (permalink / raw) To: Kito Cheng; +Cc: Andrew Pinski, gcc-patches 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 > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-10-06 2:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-05 3:54 [PATCH] RISC-V: Default to tuning for the thead-c906 Palmer Dabbelt 2022-10-05 5:56 ` Andrew Pinski 2022-10-06 1:58 ` Kito Cheng 2022-10-06 2:45 ` Andrew Waterman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).