| 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. */
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. (aarch64_override_options): Update comment about default cpu.