[PATCH, ARM] PR68674 Fix LTO support for neon builtin and error catching (ping)

[Kyrill Tkachov]

Hi Christian,

On 07/01/16 10:31, Christian Bruel wrote:
>
> It seems that changing the arm_cpu_builtins code in arm-c.c to do:
>     cpp_undef (pfile, "__ARM_NEON_FP");
>     if (TARGET_NEON_FP)
>       builtin_define_with_int_value ("__ARM_NEON_FP", TARGET_NEON_FP);
>
> instead of:
>     if (TARGET_NEON_FP)
>       builtin_define_with_int_value ("__ARM_NEON_FP", TARGET_NEON_FP);
>     else
>       cpp_undef (pfile, "__ARM_NEON_FP");
>
> fixes this. I guess we should be undefing any macros before redefining them.I
>
>>> I don't know, If we are going from v7 to v8, the __ARM_NEON_FP value indeed changes. The question is whether we want to hide this to the user ?
>> Well, the user explicitly changed the fpu level with the pragma in the testcase, which rightly has the effect of
>> changing the value of some predefines. I don't think warning is appropriate here, as long as the predefine
>> has the right value in the right pragma scope.
>>
> note that the issue can be reproduced on the current trunc with the default configure with
>
> arm-none-eabi-gcc -mfloat-abi=softfp  -mfpu=neon-fp-armv8
> --------------------------------------------------
> #pragma GCC target ("fpu=neon")
> -------------------------------------------------
> that gives:
>
> warning: "__ARM_NEON_FP" redefined
>  <built-in>: note: this is the location of the previous definition
>
> So I agree, the warning seems intuitively useless, but I'd just need to mention :
>
> iso/iec 99 A.3 and 6.10 (Preprocessing directives) #pragma is a preprocessor directive
> and iso/iec 99 6.10.3 (Macro Replacement)
>
> also, you fix is slightly wrong, since the warning should be kept for other explicit #define user redefinitions. And I think we shouldn't use cpp_undef for that. It's better to use NODE_CONDITIONAL flags on the macro identifier, since 
> this is what it is.
>
> I'd like to send a patch for this separately, so you can continue reviewing this one independently of the decision
>

Sure, I see you already filed PR target/69180 for this. Thanks.
I'm not sure whether it's valid for the user to redefine ACLE macros like this anyway, but if it's possible
to warn them about this and not warn for this implicit redefinition done by the compiler itself, then that
would be desirable.


> note that for the time being to make the patches independent the current test can be fixed with
>
> #if __ARM_ARCH==8
> #pragma GCC target ("fpu=neon-fp-armv8")
> #else
> #pragma GCC target ("fpu=neon")
> #endif
>

We agree that it's a legitimate bug, so I think the error should remain.
After all, silencing it by tweaking the testcase doesn't make the underlying bug go away.
Depending on how complicated it would be to fix PR 69180 maybe we should apply a fix
for that first and then apply this patch?

> thanks for catching this with the aarch32 v8 validation. I'll add this option to my validation scripts.
>
> Cheers
>

Another nit I spotted in the testcases:

Index: gcc/testsuite/gcc.target/arm/lto/pr65837-attr_0.c
===================================================================
--- gcc/testsuite/gcc.target/arm/lto/pr65837-attr_0.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/lto/pr65837-attr_0.c	(working copy)
@@ -0,0 +1,17 @@
+/* { dg-lto-do run } */
+/* { dg-require-effective-target arm_neon_ok } */

If you're intending to run the testcases then  you should use the arm_neon_hw effective target.


On the subject of initialising the builtins unconditionally and the memory/performance impact:
Do you think as a first step it would be a good idea to not initialise the builtins if the configuration
is not linkable with any possible NEON configuration?
For example, -mfloat-abi=soft. We're not allowed to link such modules with any possible module that uses NEON
anyway because of the ABI, so at least users of -mfloat-abi=soft wouldn't have to sit through the NEON builtins
initialisation.

Thanks,
Kyrill