Re: [testsuite][ARM target attributes] Fix effective_target tests

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

On 18 December 2015 at 15:16, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
> 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.
>
Indeed, I forgot it.

Here is a new version of the patch with a few words added to document
this function.
I did not modify the testcase after Christian's comments and
PR68934: my understanding is that the testscase are valid after
all and Christian is working on fixing the ICE.

2016-01-04  Christophe Lyon  <christophe.lyon@linaro.org>

    * doc/sourcebuild.texi (arm_crypto_pragma_ok): Document new entry.
    (arm_fp_ok): Likewise.
    (arm_fp): Likewise.
    (arm_crypto): 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.

OK?

Christophe.


> 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.
>>>>>>>>
>>>>>>>>
>