[testsuite][ARM target attributes] Fix effective_target tests
Kyrill Tkachov
kyrylo.tkachov@foss.arm.com
Fri Dec 18 14:16:00 GMT 2015
Hi Christophe,
On 17/12/15 22:17, Christophe Lyon wrote:
> Hi,
>
> Here is an updated version of this patch.
> I did test it with
> -mthumb/-march=armv8-a/-mfpu=crypto-neon-fp-armv8/-mfloat-abi=hard in
> addition to my usual set of options.
>
> Compared to the previous version:
> - I added some doc in sourcebuild.texi
> - I no longer modify arm_vfp_ok...
> - I replaced all uses of arm_vfp with the new arm_fp because I found
> that the existing tests do not actually need to pass -mfpu=vfp: this
> is implicitly set as the default when using -mfloat-abi={softfp|hard}
> - I chose not to remove arm_vfp_ok because we may need it in the
> future, if a test really needs vfp (as opposed to neon for instance)
> - in gcc.target/arm/attr-crypto.c I force the initial fpu to be vfp
> via pragma instead, so that the next pragma fpu
> fpu=crypto-neon-fp-armv8 is always compatible, regardless of the
> command-line options/default fpu
> - same for attr-neon2.c and attr-neon3.c
> - I updated cmp-2.c, unsigned-float.c, vfp-1.c, vfp-ldmdbd.c,
> vfp-ldmdbs.c, vfp-ldmiad.c, vfp-ldmias.c, vfp-stmdbd.c, vfp-stmdbs.c,
> vfp-stmiad.c, vfp-stmias.c, vnmul-[1234].c to use the new arm_fp
> effective target instead of arm_vfp. This is so that they don't need
> to use -mfpu=vfp and can use the new dg-add-options arm_fp
>
> The validation results show (in addition to what I originally reported):
> - attr-crypto.c and attr-neon3.c now ICE in some cases. This is PR68895.
> - depending on the GCC configuration (e.g. --with-fpu=neon)
> attr-neon3.c may fail. This is PR68896.
>
> OK?
Thanks for following up on this.
I think you also need to document the new arm_crypto_pragma_ok.
Kyrill
> Christophe
>
> 2015-12-17 Christophe Lyon <christophe.lyon@linaro.org>
>
> * doc/sourcebuild.texi (arm_fp_ok): Document new entry.
> (arm_fp): Likewise.
> * lib/target-supports.exp
> (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_fp_ok effective
> target instead. Force initial fpu to vfp.
> * gcc.target/arm/attr-neon-builtin-fail.c: Do not force
> -mfloat-abi=softfp, use arm_fp_ok 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. Force initial fpu to vfp.
> * gcc.target/arm/attr-neon3.c: Likewise.
> * gcc.target/arm/cmp-2.c: Use arm_fp_ok effective target instead of
> arm_vfp_ok.
> * gcc.target/arm/unsigned-float.c: Likewise.
> * gcc.target/arm/vfp-1.c: Likewise.
> * gcc.target/arm/vfp-ldmdbd.c: Likewise.
> * gcc.target/arm/vfp-ldmdbs.c: Likewise.
> * gcc.target/arm/vfp-ldmiad.c: Likewise.
> * gcc.target/arm/vfp-ldmias.c: Likewise.
> * gcc.target/arm/vfp-stmdbd.c: Likewise.
> * gcc.target/arm/vfp-stmdbs.c: Likewise.
> * gcc.target/arm/vfp-stmiad.c: Likewise.
> * gcc.target/arm/vfp-stmias.c: Likewise.
> * gcc.target/arm/vnmul-1.c: Likewise.
> * gcc.target/arm/vnmul-2.c: Likewise.
> * gcc.target/arm/vnmul-3.c: Likewise.
> * gcc.target/arm/vnmul-4.c: Likewise.
>
>
>
> On 10 December 2015 at 20:52, Christophe Lyon
> <christophe.lyon@linaro.org> wrote:
>> 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.
>>>>>>>
More information about the Gcc-patches
mailing list