Message ID | 530C6B89.10409@arm.com |
---|---|
State | New |
Headers | show |
On 25/02/14 10:08, Kyrill Tkachov wrote: > > 2014-02-25 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > * config.gcc (aarch64*-*-*): Use ISA flags from aarch64-arches.def. > Do not define target_cpu_default2 to generic. > * config/aarch64/aarch64.h (TARGET_CPU_DEFAULT): Use generic cpu. > * config/aarch64/aarch64.c (aarch64_override_options): Update comment. > * config/aarch64/aarch64-arches.def (armv8-a): Use generic cpu. > (aarch64_override_options): Update comment about default cpu. Apologies, the ChangeLog should not have that last line in it: 2014-02-25 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * config.gcc (aarch64*-*-*): Use ISA flags from aarch64-arches.def. Do not define target_cpu_default2 to generic. * config/aarch64/aarch64.h (TARGET_CPU_DEFAULT): Use generic cpu. * config/aarch64/aarch64.c (aarch64_override_options): Update comment. * config/aarch64/aarch64-arches.def (armv8-a): Use generic cpu.
Ping? On 25/02/14 10:08, Kyrill Tkachov wrote: > Hi all, > > The problem solved in this patch is that when gcc is configured with > --with-arch=armv8-a gcc will go into aarch64-arches.def, pick the > representative CPU (Cortex-A53 for ARMv8-A) and use that CPUs ISA flags. Now > that we specified that Cortex-A53 has CRC and crypto though, this means that gcc > will choose by default to enable CRC and Crypto. > > What it should be doing though is to use the 4th field in the AARCH64_ARCH macro > that specifies the ISA flags implied by the architecture. This patch does that > by looking in aarch64-arches.def and extracting the 4th field appropriately and > using that as the ext_mask when processing a --with-arch option. > > Furthermore, if no --with-arch or --with-cpu directives are specified config.gcc > will set TARGET_DEFAULT_CPU to TARGET_CPU_generic. What it should be doing, is > leaving it undefined so that the backend in aarch64.h can define its own default > with the correct ISA options (currently we have this scheme where the > TARGET_CPU_<core> is encoded in the first 6 bits of TARGET_DEFAULT_CPU and the > ISA flags are encoded in the upper part of it. We should clean that up in the > next release). Before this patch, the code in aarch64.h that does that > initialisation was never even exercised because TARGET_CPU_DEFAULT was always > defined by config.gcc no matter what! config.gcc defined it as > TARGET_CPU_generic but without encoding the appropriate ISA flags in the upper > bits, leading to a cpu configured without fp or simd. > > After a discussion with Richard, this patch sets the default CPU (if no > -mcpu,-march,--with-cpu,--with-arch is given) to be generic+fp+simd. The generic > CPU already schedules like the Cortex-A53, so it should give a decent generic > tuning. > > This patch should improve the current situation a bit. > With this patch: > - If --with-arch=armv8-a is specified we will use generic+fp+simd as the CPU > (without the patch it's cortex-a53+fp+simd+crc+crypto) > - If no arch or cpu options specified anywhere, we will use the generic+fp+simd > CPU (without the patch it would be just generic) > > Tested aarch64-none-elf on a model and checked the .cpu directive in the > generated assembly for a variety of --with-cpu, --with-arch combinations. > > I'm proposing this patch as an alternative to > http://gcc.gnu.org/ml/gcc-patches/2014-02/msg01072.html. > > Ok for trunk? > > Thanks, > Kyrill > > 2014-02-25 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > * config.gcc (aarch64*-*-*): Use ISA flags from aarch64-arches.def. > Do not define target_cpu_default2 to generic. > * config/aarch64/aarch64.h (TARGET_CPU_DEFAULT): Use generic cpu. > * config/aarch64/aarch64.c (aarch64_override_options): Update comment. > * config/aarch64/aarch64-arches.def (armv8-a): Use generic cpu.
Ping^2 http://gcc.gnu.org/ml/gcc-patches/2014-02/msg01487.html Kyrill On 05/03/14 09:37, Kyrill Tkachov wrote: > Ping? > > On 25/02/14 10:08, Kyrill Tkachov wrote: >> Hi all, >> >> The problem solved in this patch is that when gcc is configured with >> --with-arch=armv8-a gcc will go into aarch64-arches.def, pick the >> representative CPU (Cortex-A53 for ARMv8-A) and use that CPUs ISA flags. Now >> that we specified that Cortex-A53 has CRC and crypto though, this means that gcc >> will choose by default to enable CRC and Crypto. >> >> What it should be doing though is to use the 4th field in the AARCH64_ARCH macro >> that specifies the ISA flags implied by the architecture. This patch does that >> by looking in aarch64-arches.def and extracting the 4th field appropriately and >> using that as the ext_mask when processing a --with-arch option. >> >> Furthermore, if no --with-arch or --with-cpu directives are specified config.gcc >> will set TARGET_DEFAULT_CPU to TARGET_CPU_generic. What it should be doing, is >> leaving it undefined so that the backend in aarch64.h can define its own default >> with the correct ISA options (currently we have this scheme where the >> TARGET_CPU_<core> is encoded in the first 6 bits of TARGET_DEFAULT_CPU and the >> ISA flags are encoded in the upper part of it. We should clean that up in the >> next release). Before this patch, the code in aarch64.h that does that >> initialisation was never even exercised because TARGET_CPU_DEFAULT was always >> defined by config.gcc no matter what! config.gcc defined it as >> TARGET_CPU_generic but without encoding the appropriate ISA flags in the upper >> bits, leading to a cpu configured without fp or simd. >> >> After a discussion with Richard, this patch sets the default CPU (if no >> -mcpu,-march,--with-cpu,--with-arch is given) to be generic+fp+simd. The generic >> CPU already schedules like the Cortex-A53, so it should give a decent generic >> tuning. >> >> This patch should improve the current situation a bit. >> With this patch: >> - If --with-arch=armv8-a is specified we will use generic+fp+simd as the CPU >> (without the patch it's cortex-a53+fp+simd+crc+crypto) >> - If no arch or cpu options specified anywhere, we will use the generic+fp+simd >> CPU (without the patch it would be just generic) >> >> Tested aarch64-none-elf on a model and checked the .cpu directive in the >> generated assembly for a variety of --with-cpu, --with-arch combinations. >> >> I'm proposing this patch as an alternative to >> http://gcc.gnu.org/ml/gcc-patches/2014-02/msg01072.html. >> >> Ok for trunk? >> >> Thanks, >> Kyrill >> >> 2014-02-25 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> * config.gcc (aarch64*-*-*): Use ISA flags from aarch64-arches.def. >> Do not define target_cpu_default2 to generic. >> * config/aarch64/aarch64.h (TARGET_CPU_DEFAULT): Use generic cpu. >> * config/aarch64/aarch64.c (aarch64_override_options): Update comment. >> * config/aarch64/aarch64-arches.def (armv8-a): Use generic cpu. > >
On 25 February 2014 10:08, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: > Hi all, > > The problem solved in this patch is that when gcc is configured with > --with-arch=armv8-a gcc will go into aarch64-arches.def, pick the > representative CPU (Cortex-A53 for ARMv8-A) and use that CPUs ISA flags. Now > that we specified that Cortex-A53 has CRC and crypto though, this means that > gcc will choose by default to enable CRC and Crypto. > > What it should be doing though is to use the 4th field in the AARCH64_ARCH > macro that specifies the ISA flags implied by the architecture. This patch > does that by looking in aarch64-arches.def and extracting the 4th field > appropriately and using that as the ext_mask when processing a --with-arch > option. > > Furthermore, if no --with-arch or --with-cpu directives are specified > config.gcc will set TARGET_DEFAULT_CPU to TARGET_CPU_generic. What it should > be doing, is leaving it undefined so that the backend in aarch64.h can > define its own default with the correct ISA options (currently we have this > scheme where the TARGET_CPU_<core> is encoded in the first 6 bits of > TARGET_DEFAULT_CPU and the ISA flags are encoded in the upper part of it. We > should clean that up in the next release). Before this patch, the code in > aarch64.h that does that initialisation was never even exercised because > TARGET_CPU_DEFAULT was always defined by config.gcc no matter what! > config.gcc defined it as TARGET_CPU_generic but without encoding the > appropriate ISA flags in the upper bits, leading to a cpu configured without > fp or simd. > > After a discussion with Richard, this patch sets the default CPU (if no > -mcpu,-march,--with-cpu,--with-arch is given) to be generic+fp+simd. The > generic CPU already schedules like the Cortex-A53, so it should give a > decent generic tuning. > > This patch should improve the current situation a bit. > With this patch: > - If --with-arch=armv8-a is specified we will use generic+fp+simd as the CPU > (without the patch it's cortex-a53+fp+simd+crc+crypto) > - If no arch or cpu options specified anywhere, we will use the > generic+fp+simd CPU (without the patch it would be just generic) > > Tested aarch64-none-elf on a model and checked the .cpu directive in the > generated assembly for a variety of --with-cpu, --with-arch combinations > > I'm proposing this patch as an alternative to > http://gcc.gnu.org/ml/gcc-patches/2014-02/msg01072.html. - if test x$target_cpu_cname = x + if test x"$target_cpu_cname" != x I think the addition of quoting here is orthogonal to the issue you are fixing. There are several other references to target_cpu_cname in config.gcc none of which are quoted, so I guess either they should all be quoted, or not, and if they are it is a separate patch. That nit aside I think the rest of the patch makes sense for stage-4. Give the RM's 24 hours to comment otherwise drop the above nit and commit. Cheers /Marcus
Hi Marcus, On 11/03/14 11:25, Marcus Shawcroft wrote: > On 25 February 2014 10:08, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: >> Hi all, >> >> The problem solved in this patch is that when gcc is configured with >> --with-arch=armv8-a gcc will go into aarch64-arches.def, pick the >> representative CPU (Cortex-A53 for ARMv8-A) and use that CPUs ISA flags. Now >> that we specified that Cortex-A53 has CRC and crypto though, this means that >> gcc will choose by default to enable CRC and Crypto. >> >> What it should be doing though is to use the 4th field in the AARCH64_ARCH >> macro that specifies the ISA flags implied by the architecture. This patch >> does that by looking in aarch64-arches.def and extracting the 4th field >> appropriately and using that as the ext_mask when processing a --with-arch >> option. >> >> Furthermore, if no --with-arch or --with-cpu directives are specified >> config.gcc will set TARGET_DEFAULT_CPU to TARGET_CPU_generic. What it should >> be doing, is leaving it undefined so that the backend in aarch64.h can >> define its own default with the correct ISA options (currently we have this >> scheme where the TARGET_CPU_<core> is encoded in the first 6 bits of >> TARGET_DEFAULT_CPU and the ISA flags are encoded in the upper part of it. We >> should clean that up in the next release). Before this patch, the code in >> aarch64.h that does that initialisation was never even exercised because >> TARGET_CPU_DEFAULT was always defined by config.gcc no matter what! >> config.gcc defined it as TARGET_CPU_generic but without encoding the >> appropriate ISA flags in the upper bits, leading to a cpu configured without >> fp or simd. >> >> After a discussion with Richard, this patch sets the default CPU (if no >> -mcpu,-march,--with-cpu,--with-arch is given) to be generic+fp+simd. The >> generic CPU already schedules like the Cortex-A53, so it should give a >> decent generic tuning. >> >> This patch should improve the current situation a bit. >> With this patch: >> - If --with-arch=armv8-a is specified we will use generic+fp+simd as the CPU >> (without the patch it's cortex-a53+fp+simd+crc+crypto) >> - If no arch or cpu options specified anywhere, we will use the >> generic+fp+simd CPU (without the patch it would be just generic) >> >> Tested aarch64-none-elf on a model and checked the .cpu directive in the >> generated assembly for a variety of --with-cpu, --with-arch combinations >> >> I'm proposing this patch as an alternative to >> http://gcc.gnu.org/ml/gcc-patches/2014-02/msg01072.html. > - if test x$target_cpu_cname = x > + if test x"$target_cpu_cname" != x > > I think the addition of quoting here is orthogonal to the issue you > are fixing. There are several other references to target_cpu_cname in > config.gcc none of which are quoted, so I guess either they should all > be quoted, or not, and if they are it is a separate patch. Perhaps I should have commented on this. This change is not orthogonal. When I initially wrote it as " if test x$target_cpu_cname != x" the script complained of an error and happily ignored that line, giving the wrong value to target_cpu_default2 on the line below! "config.gcc: line 4065: test: too many arguments" If I quote it, it works fine. I suspect it's because of spaces introduced into target_cpu_cname earlier, since target_cpu_cname has the format "TARGET_CPU_$base_id | $ext_mask" from earlier, but I'm not sure. Kyrill > That nit aside I think the rest of the patch makes sense for stage-4. > Give the RM's 24 hours to comment otherwise drop the above nit and > commit. > > Cheers > /Marcus >
On 11 March 2014 11:48, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: >> - if test x$target_cpu_cname = x >> + if test x"$target_cpu_cname" != x >> >> I think the addition of quoting here is orthogonal to the issue you >> are fixing. There are several other references to target_cpu_cname in >> config.gcc none of which are quoted, so I guess either they should all >> be quoted, or not, and if they are it is a separate patch. > > > Perhaps I should have commented on this. > This change is not orthogonal. > When I initially wrote it as " if test x$target_cpu_cname != x" the script > complained of an error and happily ignored that line, giving the wrong value > to target_cpu_default2 on the line below! > > "config.gcc: line 4065: test: too many arguments" > > If I quote it, it works fine. I suspect it's because of spaces introduced > into target_cpu_cname earlier, since target_cpu_cname has the format > "TARGET_CPU_$base_id | $ext_mask" from earlier, but I'm not sure. For the benefit of the list, Kyrill and I just discussed the need for quoting on target_cpu_cname. In the aarch64 path the value constructed in target_cpu_cname is a '|' expression ripped from the table in aarch64-option-extensions.def hence the quoting on the argument to the test invocation is required to prevent the shell interpreting the '|'. The following use of the variable on the RHS of an assignment does not require additional quoting. I'm happy that the patch makes sense and should be committed. /Marcus
commit 918bb596fde24640a68e5d1febd6d94d6acbd9ab Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com> Date: Mon Feb 24 09:10:03 2014 +0000 [AArch64] Fix default CPU options diff --git a/gcc/config.gcc b/gcc/config.gcc index 2156640..89da61b 100644 --- a/gcc/config.gcc +++ b/gcc/config.gcc @@ -3391,6 +3391,11 @@ case "${target}" in ${srcdir}/config/aarch64/$def | \ sed -e 's/^[^,]*,[ ]*//' | \ sed -e 's/,.*$//'` + # Extract the architecture flags from aarch64-arches.def + ext_mask=`grep "^$pattern(\"$base_val\"," \ + ${srcdir}/config/aarch64/$def | \ + sed -e 's/)$//' | \ + sed -e 's/^.*,//'` else base_id=`grep "^$pattern(\"$base_val\"," \ ${srcdir}/config/aarch64/$def | \ @@ -4052,10 +4057,8 @@ esac target_cpu_default2= case ${target} in aarch64*-*-*) - if test x$target_cpu_cname = x + if test x"$target_cpu_cname" != x then - target_cpu_default2=TARGET_CPU_generic - else target_cpu_default2=$target_cpu_cname fi ;; diff --git a/gcc/config/aarch64/aarch64-arches.def b/gcc/config/aarch64/aarch64-arches.def index 5028f61..4b796d8 100644 --- a/gcc/config/aarch64/aarch64-arches.def +++ b/gcc/config/aarch64/aarch64-arches.def @@ -26,4 +26,4 @@ this architecture. ARCH is the architecture revision. FLAGS are the flags implied by the architecture. */ -AARCH64_ARCH("armv8-a", cortexa53, 8, AARCH64_FL_FOR_ARCH8) +AARCH64_ARCH("armv8-a", generic, 8, AARCH64_FL_FOR_ARCH8) diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 21054e5..624a7bb 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -5307,7 +5307,7 @@ aarch64_override_options (void) /* If the user did not specify a processor, choose the default one for them. This will be the CPU set during configuration using - --with-cpu, otherwise it is "cortex-a53". */ + --with-cpu, otherwise it is "generic". */ if (!selected_cpu) { selected_cpu = &all_cores[TARGET_CPU_DEFAULT & 0x3f]; diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h index 13c424c..d0463be 100644 --- a/gcc/config/aarch64/aarch64.h +++ b/gcc/config/aarch64/aarch64.h @@ -472,10 +472,10 @@ enum target_cpus TARGET_CPU_generic }; -/* If there is no CPU defined at configure, use "cortex-a53" as default. */ +/* If there is no CPU defined at configure, use generic as default. */ #ifndef TARGET_CPU_DEFAULT #define TARGET_CPU_DEFAULT \ - (TARGET_CPU_cortexa53 | (AARCH64_CPU_DEFAULT_FLAGS << 6)) + (TARGET_CPU_generic | (AARCH64_CPU_DEFAULT_FLAGS << 6)) #endif /* The processor for which instructions should be scheduled. */