Hi Marcus,
On 11/03/14 11:25, Marcus Shawcroft wrote:
On 25 February 2014 10:08, Kyrill Tkachov <kyrylo.tkac...@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
