Re: [PATCH 00/30] [ARM] Reworking the -mcpu, -march and -mfpu options

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On 10 June 2017 at 01:27, Richard Earnshaw (lists)
<Richard.Earnshaw@arm.com> wrote:
> On 09/06/17 23:45, Christophe Lyon wrote:
>> Hi Richard,
>>
>>
>> On 9 June 2017 at 14:53, Richard Earnshaw <Richard.Earnshaw@arm.com> wrote:
>>>
>>> During the ARM BoF at the Cauldron last year I mentioned that I wanted
>>> to rework the way GCC on ARM handles the command line options.  The
>>> problem was that most users, and even many experts, can't remember
>>> which FPU/SIMD unit comes with which CPU and that consequently many
>>> users were inadvertenly generating sub-optimal code for their system.
>>>
>>> This patch series implements the proposed change and provides support
>>> for a generic way of adding optional features to architectures and CPU
>>> names.  The documentation patches at the end of the series explain the
>>> new syntax, so I won't repeat all that here.  Suffice to say here that
>>> the result is that the -mfpu option now defaults to 'auto', which
>>> allows the compiler to infer the floating-point and simd options from
>>> the CPU/architecture options and that these options can normally be
>>> expressed in a context-specific manner like +simd or +fp without
>>> having to know precisely which variant is implemented.  Long term I'd
>>> like to deprecate -mfpu and entirely move over to the new syntax; but
>>> it's too early to start that process now.
>>>
>>> All the patches in the series should build a working basic compiler,
>>> but the multilib selection will not work correctly until the relevant
>>> patches towards the end are applied.  It is not really feasible to
>>> retain that functionality without collapsing too many of the patches
>>> together into one hunk.  It's also possible that some tests in the
>>> testsuite may exhibit transient misbehaviour, but there should be no
>>> regressions by the end of the sequence (some tests no-longer run in
>>> the default configurations because the default CPU does not have
>>> floating-point support).
>>>
>>> Just two patches are to the generic code, but both are fairly trivial.
>>> One permits the sbitmap code to be used in the driver programs and the
>>> other provides a way of escaping the meta-character in some multilib
>>> reuse strings.
>>>
>>> I won't apply any of this series until those two patches have been
>>> approved, and I won't commit anything before the middle of next week
>>> even then.  This is a fairly complex change and it deserves some time
>>> for people to comment before committing.
>>>
>>> R.
>>>
>>> Richard Earnshaw (30):
>>>   [arm] Use strings for -march, -mcpu and -mtune options
>>>   [arm] Rewrite -march and -mcpu options for passing to the assembler
>>>   [arm] Don't pass -mfpu=auto through to the assembler.
>>>   [arm] Allow +opt on arbitrary cpu and architecture specifications
>>>   [arm] Add architectural options
>>>   [arm] Add default FPUs for CPUs.
>>>   [build] Make sbitmap code available to the driver programs
>>>   [arm] Split CPU, architecture and tuning data tables.
>>>   [ARM] Move cpu and architecture option name parsing code to
>>>     arm-common.c
>>>   [arm] Use standard option parsing code for detecting thumb-only
>>>     targets
>>>   [arm] Allow CPU and architecture extensions to be defined as aliases
>>>   [arm] Allow new extended syntax CPU and architecture names during
>>>     configure
>>>   [arm] Force a CPU default in the config args defaults list.
>>>   [arm] Generate a canonical form for -march
>>>   [arm] Make -mfloat-abi=softfp work when there are no FPU instructions
>>>   [arm] Update basic multilib configuration
>>>   [arm] Make 'auto' the default FPU selection option.
>>>   [arm] Rewrite t-aprofile using new selector methodology
>>>   [arm] Explicitly set .fpu in cmse_nonsecure_call.S
>>>   [genmultilib] Allow explicit periods to be escaped in MULTILIB_REUSE
>>>   [arm][testsuite] Use -march=armv7-a+fp when testing hard-float ABI.
>>>   [arm] Rewrite t-rmprofile multilib specification
>>>   [arm][rtems] Update t-rtems for new option framework
>>>   [arm][linux-eabi] Ensure all multilib variables are reset
>>>   [arm][phoenix] reset all multilib variables
>>>   [arm] Rework multlib builds for symbianelf
>>>   [arm][fuchsia] Rework multilib support
>>>   [arm] Add a few missing architecture extension options.
>>>   [arm][doc] Document new -march= syntax.
>>>   [arm][doc] Document changes to -mcpu, -mtune and -mfpu.
>>>
>>>  gcc/Makefile.in                           |    2 +-
>>>  gcc/common/config/arm/arm-common.c        |  651 +++++++-
>>>  gcc/config.gcc                            |   17 +-
>>>  gcc/config/arm/arm-builtins.c             |    4 +-
>>>  gcc/config/arm/arm-cpu-cdata.h            | 2444 +++++++++++++++++++++++------
>>>  gcc/config/arm/arm-cpu-data.h             | 1410 ++---------------
>>>  gcc/config/arm/arm-cpu.h                  |   38 +
>>>  gcc/config/arm/arm-cpus.in                |  237 ++-
>>>  gcc/config/arm/arm-isa.h                  |   20 +-
>>>  gcc/config/arm/arm-protos.h               |   56 +-
>>>  gcc/config/arm/arm-tables.opt             |   21 +-
>>>  gcc/config/arm/arm.c                      |  337 ++--
>>>  gcc/config/arm/arm.h                      |   75 +-
>>>  gcc/config/arm/arm.opt                    |   15 +-
>>>  gcc/config/arm/bpabi.h                    |    4 -
>>>  gcc/config/arm/elf.h                      |    6 +-
>>>  gcc/config/arm/linux-elf.h                |    3 -
>>>  gcc/config/arm/netbsd-elf.h               |    4 -
>>>  gcc/config/arm/parsecpu.awk               |  295 +++-
>>>  gcc/config/arm/t-aprofile                 |  200 +--
>>>  gcc/config/arm/t-arm-elf                  |  173 +-
>>>  gcc/config/arm/t-fuchsia                  |   33 +
>>>  gcc/config/arm/t-linux-eabi               |    4 +
>>>  gcc/config/arm/t-multilib                 |  126 +-
>>>  gcc/config/arm/t-phoenix                  |   20 +-
>>>  gcc/config/arm/t-rmprofile                |  147 +-
>>>  gcc/config/arm/t-rtems                    |   49 +-
>>>  gcc/config/arm/t-symbian                  |   34 +-
>>>  gcc/config/arm/vxworks.h                  |    2 -
>>>  gcc/doc/fragments.texi                    |   10 +-
>>>  gcc/doc/invoke.texi                       |  371 ++++-
>>>  gcc/genmultilib                           |    4 +-
>>>  gcc/testsuite/gcc.dg/pr59418.c            |    2 +-
>>>  gcc/testsuite/gcc.target/arm/multilib.exp |  685 ++++++++
>>>  gcc/testsuite/gcc.target/arm/pr51915.c    |    2 +-
>>>  gcc/testsuite/gcc.target/arm/pr52006.c    |    2 +-
>>>  gcc/testsuite/gcc.target/arm/pr53187.c    |    2 +-
>>>  libgcc/config/arm/cmse_nonsecure_call.S   |    8 +
>>>  38 files changed, 5073 insertions(+), 2440 deletions(-)
>>>  create mode 100644 gcc/config/arm/t-fuchsia
>>>  create mode 100644 gcc/testsuite/gcc.target/arm/multilib.exp
>>>
>>> ----------------2.7.4--
>>>
>>
>> I wanted to run a validation with the full series applied as one patch
>> over r249050,
>> so I just downloaded all the patches, concatenated them in order, but the result
>> fails to apply. (conflicts with arm-cpu-cdata.h, arm-cpus.in,
>> t-aprofile, t-rmprofile)
>>
>> Am I missing something?
>>
>> Thanks,
>>
>> Christophe
>>
>
> I really appreciate you trying to do this...
>
> I rebased the patch series sometime on Tuesday/Wednesday just before Jim
> Wilson committed his falkor change; you'll need to back out to just
> before r248944.  I can't think of anything else that might have just
> gone in that might conflict.  For arm-cpu-cdata.h (and the other
> auto-generated files, like arm-cpu-data.h) you can just delete the file
> and it will be rebuilt automatically, the others really do need the
> conflict to be resolved.
>
> Obviously I'll rebase again just before the commit, but propagating such
> changes through the patch series is quite tedious and I don't want to do
> it any more than necessary :-).
>
> It would be easier if the generated files weren't checked in to the
> repository...
>
> R.

Hi Richard,

Starting a validation of the whole patch against r248942 did work, thanks.

The results are here:
http://people.linaro.org/~christophe.lyon/cross-validation/gcc-test-patches/248942-rework-cpu-arch-fpu.patch/report-build-info.html

Regressions are detected in many configurations, unfortunately.
Some may be caused by the fact that I've upgraded to dejagnu-1.6+ for
my testing,
and thus "multilib flags" are now prepended rather than appended, but
I don't think that's the majority.

To help you understand/group all the reports:
- all "arm-none-linux-gnueabi" with REGRESSED now have:
FAIL: gcc.target/arm/neon-thumb2-move.c (test for excess errors)
FAIL: gcc.target/arm/no-volatile-in-it.c scan-assembler-not ldrgt
- same for the 2 configs arm-none-eabi --with-cpu=cortex-a9

- all "arm-none-linux-gnueabihf" and "armeb-none-linux-gnueabihf" with
REGRESSED now have:
FAIL: gcc.target/arm/no-volatile-in-it.c scan-assembler-not ldrgt

- arm-none-eabi --with-cpu=cortex-m3 now has:
FAIL:
    gcc.target/arm/no-volatile-in-it.c scan-assembler-not ldrgt
    gcc.target/arm/thumb2-slow-flash-data-2.c (test for excess errors)
    gcc.target/arm/thumb2-slow-flash-data-3.c (test for excess errors)
    gcc.target/arm/thumb2-slow-flash-data-4.c (test for excess errors)
    gcc.target/arm/thumb2-slow-flash-data-5.c (test for excess errors)
UNRESOLVED:
    gcc.target/arm/thumb2-slow-flash-data-4.c scan-assembler-times #1\\.0e\\+0 3
    gcc.target/arm/thumb2-slow-flash-data-5.c scan-assembler-not #1\\.0e\\+0

- The 2 cells with "BIG-REG" mean regressions where detected, but the report
is >100kb so it was attached as a .xz file, click on 'BIG-REG" to download it.

- the cells with "BETTER" can be questionable: it means no new failure
appeared, but PASS -> UNSUPPORTED is considered as "better". Here
we have cases with several thousands (!) of tests becoming unsupported,
which looks a bit suspicious.

Among other things, I've noticed that when passing -march=armv5t,
arm_neon_ok fails:
xgcc -march=armv5t -fno-diagnostics-show-caret
-fdiagnostics-color=never -mfpu=neon -mfloat-abi=softfp -march=
armv7-a -c -o arm_neon_ok15574.o arm_neon_ok15574.c
arm_neon_ok15574.c:9:4: error: #error Architecture does not support NEON.
compiler exited with status 1

I'd expect -march=armv7-a to allow to use -mfpu=neon?

Regarding the new multilib.exp tests introduced by the patch, my testing
did not run it because I have no configuration setting aprofile or rmprofile.
Maybe it's time I add one, or update the existing ones.
I didn't see any message "skipping multilib tests due to
multilib_flags setting",
but I guess that's because my runs are not verbose enough. That being said,
I'm not sure how the multilib_flags are set in board_info? Is that derived from
RUNTESTFLAGS?

>From this report page, you can also click on "sum" and "log" (when
present) to download the gcc.sum or gcc.log files.

Feel free to ask more details if I haven't been clear enough.

Thanks,

Christophe