Message ID | 568E5548.6060207@foss.arm.com |
---|---|
State | New |
Headers | show |
On 01/07/2016 01:08 PM, Kyrill Tkachov wrote: > 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. yes that should be invalid for a user to redefine them, so we must warn for -D and #define, but not #pragma. I'm testing a patch for this. > >> 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? OK we should proceed in this order, pr69180 should not be complicated, so hold on for it. > >> 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. ok thanks > > > 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. good idea. > Thanks, > Kyrill >
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