On 10 December 2015 at 14:14, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
On 10/12/15 13:04, Christophe Lyon wrote:
On 10 December 2015 at 13:30, Kyrill Tkachov <kyrylo.tkachov@arm.com>
wrote:
Hi Christophe,
On 08/12/15 11:18, Christophe Lyon wrote:
On 8 December 2015 at 11:50, Kyrill Tkachov <kyrylo.tkachov@arm.com>
wrote:
Hi Christophe,
On 27/11/15 13:00, Christophe Lyon wrote:
Hi,
After the recent commits from Christian adding target attributes
support for ARM FPU settings, I've noticed that some of the tests
were failing because of incorrect assumptions wrt to the default
cpu/fpu/float-abi of the compiler.
This patch fixes the problems I've noticed in the following way:
- do not force -mfloat-abi=softfp in dg-options, to avoid conflicts
when gcc is configured --with-float=hard
- change arm_vfp_ok such that it tries several -mfpu/-mfloat-abi
flags, checks that __ARM_FP is defined and __ARM_NEON_FP is not
defined
- introduce arm_fp_ok, which is similar but does not enforce fpu
setting
- add a new effective_target: arm_crypto_pragma_ok to check that
setting this fpu via a pragma is actually supported by the current
"multilib". This is different from checking the command-line option
because the pragma might conflict with the command-line options in
use.
The updates in the testcases are as follows:
- attr-crypto.c, we have to make sure that the defaut fpu does not
conflict with the one forced by pragma. That's why I use the arm_vfp
options/effective_target. This is needed if gcc has been configured
--with-fpu=neon-fp16, as the pragma fpu=crypto-neon-fp-armv8 would
conflict.
- attr-neon-builtin-fail.c: use arm_fp to force the appropriate
float-abi setting. Enforcing fpu is not needed here.
- attr-neon-fp16.c: similar, I also removed arm_neon_ok since it was
not necessary to make the test pass in my testing. On second thought,
I'm wondering whether I should leave it and make the test unsupported
in more cases (such as when forcing -march=armv5t, although it does
pass with this patch)
- attr-neon2.c: use arm_vfp to force the appropriate float-abi
setting. Enforcing mfpu=vfp is needed to avoid conflict with the
pragma target fpu=neon (for instance if the toolchain default is
neon-fp16)
- attr-neon3.c: similar
Tested on a variety of configurations, see:
http://people.linaro.org/~christophe.lyon/cross-validation/gcc-test-patches/230929-target-attr/report-build-info.html
Note that the regressions reported fall into 3 categories:
- when forcing march=armv5t: tests are now unsupported because I
modified arm_crypto_ok to require arm_v8_neon_ok instead of arm32.
- the warning reported by attr-neon-builtin-fail.c moved from line 12
to 14 and is thus seen as a regression + one improvement
- finally, attr-neon-fp16.c causes an ICE on armeb compilers, for
which I need to post a bugzilla.
TBH, I'm a bit concerned by the complexity of all these multilib-like
conditions. I'm confident that I'm still missing some combinations :-)
And with new target attributes coming, new architectures etc... all
this logic is likely to become even more complex.
That being said, OK for trunk?
I'd like us to clean up and reorganise the gcc.target/arm testsuite
at some point to make it more robust with respect to the tons of
multilib
options and configurations we can have for arm. It's becoming very
frustrating
to test feature-specific stuff :(
This is ok in the meantime.
Sorry for the delay.
Thanks for the approval, glad to see I'm not the only one willing to see
improvements in this area :)
Committed as r231403.
With this patch we're seeing legitimate PASSes go to NA.
For example:
gcc.target/arm/vfp-1.c
gcc.target/arm/vfp-ldmdbs.c
and other vfp tests in arm.exp.
This is, for example, for the
variant-mthumb/-march=armv8-a/-mfpu=crypto-neon-fp-armv8/-mfloat-abi=hard
Hmm I'm attempting to cover such a configuration in my matrix of
validations:
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/231403/report-build-info.html
The difference is that I use similar flags at GCC configure time, while
you
override them when running the testsuite:
--target arm-none-linux-gnueabihf --with-mode=thum --with-cpu=cortex-a57
--with-fpu=crypto-neon-fp-armv8
I this case, I do not see the regressions.
My gcc is arm-none-eabi configured with
--with-float=hard --with-cpu=cortex-a15 --with-fpu=neon-vfpv4
--with-float=hard
I suspect under your new predicates they'd need to be changed to check
for
arm_fp_ok rather than arm_vfp_ok.
Probably, yes.
In the test log where it checks the effective target it fails due to:
arm_vfp_ok27268.c:6:4: error: #error __ARM_NEON_FP defined
it's compiling the check with
-mthumb -march=armv8-a -mfpu=crypto-neon-fp-armv8 -mfloat-abi=hard -o
arm_vfp_ok27268.o arm_vfp_ok27268.c
We want to avoid removing any PASSes.
Can you please test some more to make sure we don't remove any legitimate
passes with your patch?
Is that the only combination in which you saw less PASSes?
That's the one that was brought to my attention, but I think the issue here
is just the
tests that check for arm_vfp_ok rather than the new arm_fp_ok and set
-mfpu=vfp explicitly.
They appear unsupported if testing with an explicit neon option in mfpu, I
think.
Also, Ramana reminded me offline that any new predicates in
target-supports.exp
need documenting in sourcebuild.txt.
In light of that, can you please revert your patch and address the issues
above
so that we can be confident we don't lose existing legitimate test
coverage?
OK.
Sorry for not catching these sooner.
No problem, there are so many combinations.
I'm not sure how to define a reasonable set.
So, I think the action plan here is to audit the tests that need to be
changed
to arm_fp_ok and add the documentation for the new predicate checks.
I came up with a new version where I now use only the new arm_fp_ok, so
it might be easier just to update arm_vfp_ok and not introduce arm_fp_ok.
However, I'm still not happy with this version because it has problems with
a few tests that use target attributes such as attr-crypto.c, which starts with:
#pragma GCC target ("fpu=crypto-neon-fp-armv8")
This pragma fails if the compiler+options have set an incompatible
fpu before processing the pragma.
So for instance, if gcc has been configured --with-fpu=neon, compiling the test
with no fpu option fails (neon is not compatible with crypto-neon-fp-armv8
because the latter has fp16)
Now, if we use effective_target arm_vfp_ok + dg_option to force -mfpu=vfp,
we end up compiling with -mfpu=vfp and the pragma passes.
But if, in addition, we force the testflags to set -mfpu=crypto-neon-fp-armv8
as you do, the effective_target arm_vfp_ok test now fails because it is
compiled with -mfpu=vfp -mfpu=crypto-neon-fp-armv8, which defines
__ARM_NEON_FP
In short, the problem is how to make sure that the fpu setting is always
compatible with the pragma, whatever the gcc configuration and multilib
options used.
Thanks,
Christophe.
Thanks,
Kyrill
Christophe.
Kyrill
Christophe.
Thanks for handling this!
Kyrill
Christophe
2015-11-27 Christophe Lyon <christophe.lyon@linaro.org>
* lib/target-supports.exp
(check_effective_target_arm_vfp_ok_nocache): New.
(check_effective_target_arm_vfp_ok): Call the new
check_effective_target_arm_vfp_ok_nocache function.
(check_effective_target_arm_fp_ok_nocache): New.
(check_effective_target_arm_fp_ok): New.
(add_options_for_arm_fp): New.
(check_effective_target_arm_crypto_ok_nocache): Require
target_arm_v8_neon_ok instead of arm32.
(check_effective_target_arm_crypto_pragma_ok_nocache): New.
(check_effective_target_arm_crypto_pragma_ok): New.
(add_options_for_arm_vfp): New.
* gcc.target/arm/attr-crypto.c: Use arm_crypto_pragma_ok
effective
target. Do not force -mfloat-abi=softfp, use arm_vfp effective
target instead.
* gcc.target/arm/attr-neon-builtin-fail.c: Do not force
-mfloat-abi=softfp, use arm_fp effective target instead.
* gcc.target/arm/attr-neon-fp16.c: Likewise. Remove arm_neon_ok
dependency.
* gcc.target/arm/attr-neon2.c: Do not force -mfloat-abi=softfp,
use arm_vfp effective target instead.
* gcc.target/arm/attr-neon3.c: Likewise.