| Message ID | 5416C5C3.8070603@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On 15/09/14 11:56, Andrew Stubbs wrote: > On 15/09/14 10:46, Richard Earnshaw wrote: >> Hmm, I wonder if arm_override_options should reject neon + (arch < 7). > > Is this more to your taste? > Yep, that's fine. > Andrew > > P.S. arm_override_options was renamed in 2010. I'm getting old :-( R. > > > arm_neon_ok-2.patch > > > 2014-09-15 Andrew Stubbs <ams@codesourcery.com> > > * gcc/config/arm/arm.c (arm_option_override): Reject -mfpu=neon > when architecture is older than ARMv7. > > Index: gcc/config/arm/arm.c > =================================================================== > --- gcc/config/arm/arm.c (revision 215228) > +++ gcc/config/arm/arm.c (working copy) > @@ -2845,6 +2845,9 @@ > > arm_fpu_desc = &all_fpus[arm_fpu_index]; > > + if (TARGET_NEON && !arm_arch7) > + error ("target CPU does not support NEON"); > + > switch (arm_fpu_desc->model) > { > case ARM_FP_MODEL_VFP: >
On 15/09/14 14:29, Richard Earnshaw wrote:
> Yep, that's fine.
Committed, thanks.
Andrew
On Mon, Sep 15, 2014 at 11:56:03AM +0100, Andrew Stubbs wrote: > On 15/09/14 10:46, Richard Earnshaw wrote: > > Hmm, I wonder if arm_override_options should reject neon + (arch < 7). > > Is this more to your taste? Is this really such a good idea? It causes carnage throughout the testsuite if you have configured with support for Neon and the testcase is written with dg-options for a pre-armv7-a -march value. For example in: testsuite/gcc.target/arm/di-longlong64-sync-withhelpers.c Which forces -march=armv5. Perhaps you just have to fix the effective-target-ok tests - but then we lose swathes of test coverage. Thanks, James > > Andrew > > P.S. arm_override_options was renamed in 2010. > 2014-09-15 Andrew Stubbs <ams@codesourcery.com> > > * gcc/config/arm/arm.c (arm_option_override): Reject -mfpu=neon > when architecture is older than ARMv7. > > Index: gcc/config/arm/arm.c > =================================================================== > --- gcc/config/arm/arm.c (revision 215228) > +++ gcc/config/arm/arm.c (working copy) > @@ -2845,6 +2845,9 @@ > > arm_fpu_desc = &all_fpus[arm_fpu_index]; > > + if (TARGET_NEON && !arm_arch7) > + error ("target CPU does not support NEON"); > + > switch (arm_fpu_desc->model) > { > case ARM_FP_MODEL_VFP:
Maybe the original patch is better? Or maybe it should reconfigure the FPU instead of erroring out? But reconfigure it to what? Andrew
On 23/09/14 16:22, Stubbs, Andrew wrote: > Maybe the original patch is better? Or maybe it should reconfigure the FPU instead of erroring out? But reconfigure it to what? Andrew, are you still working on this? a bunch of tests on my local environment failed because of the reason James mentioned. for example gcc.target/arm/xor-and.c etc. ... gcc.target/arm/xor-and.c:1:0: error: target CPU does not support NEON ... Regards. Jiong > > Andrew > ________________________________________ > From: James Greenhalgh [james.greenhalgh@arm.com] > Sent: 23 September 2014 09:27 > To: Stubbs, Andrew > Cc: Richard Earnshaw; gcc-patches@gcc.gnu.org > Subject: Re: [arm][patch] fix arm_neon_ok check on !arm_arch7 > > On Mon, Sep 15, 2014 at 11:56:03AM +0100, Andrew Stubbs wrote: >> On 15/09/14 10:46, Richard Earnshaw wrote: >>> Hmm, I wonder if arm_override_options should reject neon + (arch < 7). >> Is this more to your taste? > Is this really such a good idea? It causes carnage throughout the > testsuite if you have configured with support for Neon and the testcase > is written with dg-options for a pre-armv7-a -march value. > > For example in: > testsuite/gcc.target/arm/di-longlong64-sync-withhelpers.c > > Which forces -march=armv5. > > Perhaps you just have to fix the effective-target-ok tests - but then > we lose swathes of test coverage. > > Thanks, > James > >> Andrew >> >> P.S. arm_override_options was renamed in 2010. >> 2014-09-15 Andrew Stubbs <ams@codesourcery.com> >> >> * gcc/config/arm/arm.c (arm_option_override): Reject -mfpu=neon >> when architecture is older than ARMv7. >> >> Index: gcc/config/arm/arm.c >> =================================================================== >> --- gcc/config/arm/arm.c (revision 215228) >> +++ gcc/config/arm/arm.c (working copy) >> @@ -2845,6 +2845,9 @@ >> >> arm_fpu_desc = &all_fpus[arm_fpu_index]; >> >> + if (TARGET_NEON && !arm_arch7) >> + error ("target CPU does not support NEON"); >> + >> switch (arm_fpu_desc->model) >> { >> case ARM_FP_MODEL_VFP: > >
On 15/10/14 17:34, Jiong Wang wrote: > On 23/09/14 16:22, Stubbs, Andrew wrote: >> Maybe the original patch is better? Or maybe it should reconfigure the >> FPU instead of erroring out? But reconfigure it to what? > > Andrew, > > are you still working on this? > a bunch of tests on my local environment failed because of the reason > James mentioned. for example gcc.target/arm/xor-and.c etc. I can do, but nobody had any suggestions of a better fix. It seems that the testsuite is broken different ways with and without neon. I get it running (and failing) NEON tests on multilibs it should not, and you get it failing non-NEON tests on toolchains configured to have NEON by default. I can apply my original patch, if that works better? Andrew
On 15/10/14 17:58, Andrew Stubbs wrote: > On 15/10/14 17:34, Jiong Wang wrote: >> On 23/09/14 16:22, Stubbs, Andrew wrote: >>> Maybe the original patch is better? Or maybe it should reconfigure the >>> FPU instead of erroring out? But reconfigure it to what? >> Andrew, >> >> are you still working on this? >> a bunch of tests on my local environment failed because of the reason >> James mentioned. for example gcc.target/arm/xor-and.c etc. > I can do, but nobody had any suggestions of a better fix. > > It seems that the testsuite is broken different ways with and without > neon. I get it running (and failing) NEON tests on multilibs it should > not, and you get it failing non-NEON tests on toolchains configured to > have NEON by default. > > I can apply my original patch, if that works better? Hi Andrew, if armv6 never co-exist with NEON, personally I think your original patch is better because TARGET_NEON generally will be used when all options are processed. any way, this needs gate keeper's approval. thanks. Regards, Jiong > > Andrew > > >
> if armv6 never co-exist with NEON, personally I think your original > patch is better > because TARGET_NEON generally will be used when all options are > processed. > > any way, this needs gate keeper's approval. Ping, Richard. Andrew
On 07/11/14 10:35, Andrew Stubbs wrote: >> if armv6 never co-exist with NEON, personally I think your original >> patch is better >> because TARGET_NEON generally will be used when all options are >> processed. >> >> any way, this needs gate keeper's approval. > > Ping, Richard. Ping.
On 14/11/14 11:12, Andrew Stubbs wrote: > On 07/11/14 10:35, Andrew Stubbs wrote: >>> if armv6 never co-exist with NEON, personally I think your original >>> patch is better >>> because TARGET_NEON generally will be used when all options are >>> processed. >>> >>> any way, this needs gate keeper's approval. >> >> Ping, Richard. > > Ping. Ping. Andrew
On Nov 26, 2014, at 4:24 AM, Andrew Stubbs <andrew_stubbs@mentor.com> wrote: > On 14/11/14 11:12, Andrew Stubbs wrote: >> On 07/11/14 10:35, Andrew Stubbs wrote: >>>> if armv6 never co-exist with NEON, personally I think your original >>>> patch is better >>>> because TARGET_NEON generally will be used when all options are >>>> processed. >>>> >>>> any way, this needs gate keeper's approval. >>> >>> Ping, Richard. >> >> Ping. > > Ping. Could you include a link or the patch. If the test suite, I'll review it if no one else steps up.
On 27/11/14 17:05, Mike Stump wrote:
> Could you include a link or the patch. If the test suite, I'll review it if no one else steps up.
The original patch is here:
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01119.html
Thanks
Andrew
On Nov 27, 2014, at 9:28 AM, Andrew Stubbs <ams@codesourcery.com> wrote: > On 27/11/14 17:05, Mike Stump wrote: >> Could you include a link or the patch. If the test suite, I'll review it if no one else steps up. > > The original patch is here: > > https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01119.html Sorry, arm people will have to approve... Many reasonable choices, yet I bet only one is best.
On 23/09/14 09:27, James Greenhalgh wrote: > On Mon, Sep 15, 2014 at 11:56:03AM +0100, Andrew Stubbs wrote: >> On 15/09/14 10:46, Richard Earnshaw wrote: >>> Hmm, I wonder if arm_override_options should reject neon + (arch < 7). >> Is this more to your taste? > Is this really such a good idea? It causes carnage throughout the > testsuite if you have configured with support for Neon and the testcase > is written with dg-options for a pre-armv7-a -march value. > > For example in: > testsuite/gcc.target/arm/di-longlong64-sync-withhelpers.c > > Which forces -march=armv5. > > Perhaps you just have to fix the effective-target-ok tests - but then > we lose swathes of test coverage. This also causes subtle Linux kernel compile failures. Over there they use make rules where they check if the compiler supports -march=armv5te and if not use -march=armv4t. With this patch if the compiler is configured with something like --with-fpu=neon the test will fail with your error message, even though the compiler supports -march=armv5te. Kyrill > > Thanks, > James > >> Andrew >> >> P.S. arm_override_options was renamed in 2010. >> 2014-09-15 Andrew Stubbs <ams@codesourcery.com> >> >> * gcc/config/arm/arm.c (arm_option_override): Reject -mfpu=neon >> when architecture is older than ARMv7. >> >> Index: gcc/config/arm/arm.c >> =================================================================== >> --- gcc/config/arm/arm.c (revision 215228) >> +++ gcc/config/arm/arm.c (working copy) >> @@ -2845,6 +2845,9 @@ >> >> arm_fpu_desc = &all_fpus[arm_fpu_index]; >> >> + if (TARGET_NEON && !arm_arch7) >> + error ("target CPU does not support NEON"); >> + >> switch (arm_fpu_desc->model) >> { >> case ARM_FP_MODEL_VFP:
On Tue, Dec 2, 2014 at 2:01 PM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote: > > On 23/09/14 09:27, James Greenhalgh wrote: >> >> On Mon, Sep 15, 2014 at 11:56:03AM +0100, Andrew Stubbs wrote: >>> >>> On 15/09/14 10:46, Richard Earnshaw wrote: >>>> >>>> Hmm, I wonder if arm_override_options should reject neon + (arch < 7). >>> >>> Is this more to your taste? >> >> Is this really such a good idea? It causes carnage throughout the >> testsuite if you have configured with support for Neon and the testcase >> is written with dg-options for a pre-armv7-a -march value. >> >> For example in: >> testsuite/gcc.target/arm/di-longlong64-sync-withhelpers.c >> >> Which forces -march=armv5. >> >> Perhaps you just have to fix the effective-target-ok tests - but then >> we lose swathes of test coverage. > > > This also causes subtle Linux kernel compile failures. > Over there they use make rules where they check if the compiler supports > -march=armv5te and if not use -march=armv4t. > With this patch if the compiler is configured with something like > --with-fpu=neon the test will fail with your error message, > even though the compiler supports -march=armv5te. I've spent some time this evening pondering over your patch. Firstly it appears that the current behaviour is going to cause more breakage than originally expected. If this is to go in we'd have a number of users having to add -mfloat-abi=soft to the command line option to ensure that -march=armv5te works just fine on the files where march=armv5te in the first places. I'm not sure that the original patch is enough. The tools have always allowed us to drop down the arch to march=armv5te along with using -mfpu=neon. We are now changing command line behaviour, so an inform in terms of diagnostics to the user would be useful as it states that we don't really have mfpu=neon generating neon code any more because of this particular case. If we are to do this then the original patch is probably not enough as it then doesn't handle the case of TARGET_VFP3 / TARGET_VFP5 / TARGET_NEON_FP16 / TARGET_FP16 / TARGET_FPU_ARMV8 etc. etc. etc. regards Ramana > > Kyrill > > > >> >> Thanks, >> James >> >>> Andrew >>> >>> P.S. arm_override_options was renamed in 2010. >>> 2014-09-15 Andrew Stubbs <ams@codesourcery.com> >>> >>> * gcc/config/arm/arm.c (arm_option_override): Reject -mfpu=neon >>> when architecture is older than ARMv7. >>> >>> Index: gcc/config/arm/arm.c >>> =================================================================== >>> --- gcc/config/arm/arm.c (revision 215228) >>> +++ gcc/config/arm/arm.c (working copy) >>> @@ -2845,6 +2845,9 @@ >>> arm_fpu_desc = &all_fpus[arm_fpu_index]; >>> + if (TARGET_NEON && !arm_arch7) >>> + error ("target CPU does not support NEON"); >>> + >>> switch (arm_fpu_desc->model) >>> { >>> case ARM_FP_MODEL_VFP: > > >
On 02/12/14 21:45, Ramana Radhakrishnan wrote: > I've spent some time this evening pondering over your patch. Firstly > it appears that the current behaviour is going to cause more breakage > than originally expected. If this is to go in we'd have a number of > users having to add -mfloat-abi=soft to the command line option to > ensure that -march=armv5te works just fine on the files where > march=armv5te in the first places. Agreed. I've just reverted the patch. > I'm not sure that the original patch is enough. > > The tools have always allowed us to drop down the arch to > march=armv5te along with using -mfpu=neon. We are now changing command > line behaviour, so an inform in terms of diagnostics to the user would > be useful as it states that we don't really have mfpu=neon generating > neon code any more because of this particular case. If we are to do > this then the original patch is probably not enough as it then doesn't > handle the case of TARGET_VFP3 / TARGET_VFP5 / TARGET_NEON_FP16 / > TARGET_FP16 / TARGET_FPU_ARMV8 etc. etc. etc. I'll take a look at those shortly. Andrew
2014-09-15 Andrew Stubbs <ams@codesourcery.com> * gcc/config/arm/arm.c (arm_option_override): Reject -mfpu=neon when architecture is older than ARMv7. Index: gcc/config/arm/arm.c =================================================================== --- gcc/config/arm/arm.c (revision 215228) +++ gcc/config/arm/arm.c (working copy) @@ -2845,6 +2845,9 @@ arm_fpu_desc = &all_fpus[arm_fpu_index]; + if (TARGET_NEON && !arm_arch7) + error ("target CPU does not support NEON"); + switch (arm_fpu_desc->model) { case ARM_FP_MODEL_VFP:
On 15/09/14 10:46, Richard Earnshaw wrote: > Hmm, I wonder if arm_override_options should reject neon + (arch < 7). Is this more to your taste? Andrew P.S. arm_override_options was renamed in 2010.