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

[Christophe Lyon <christophe.lyon@linaro.org>]

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.

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

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

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

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