On 10 December 2015 at 14:14, Kyrill Tkachov <kyrylo.tkac...@arm.com> wrote:
>
> On 10/12/15 13:04, Christophe Lyon wrote:
>>
>> On 10 December 2015 at 13:30, Kyrill Tkachov <kyrylo.tkac...@arm.com>
>> wrote:
>>>
>>> Hi Christophe,
>>>
>>>
>>> On 08/12/15 11:18, Christophe Lyon wrote:
>>>>
>>>> On 8 December 2015 at 11:50, Kyrill Tkachov <kyrylo.tkac...@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.l...@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.
>>>>>
>>>>>
>
