diff mbox

[arm] fix arm_neon_ok check on !arm_arch7

Message ID 54999C6B.9040400@codesourcery.com
State New
Headers show

Commit Message

Andrew Stubbs Dec. 23, 2014, 4:46 p.m. UTC
On 03/12/14 15:03, Andrew Stubbs wrote:
>> 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.

Or, not so shortly.

It seems that, on ARM, the arch/CPU setting is basically orthogonal to 
the FPU setting, and the compiler doesn't even try to match the one to 
the other. The assembler does the same. In fact, the testcases that 
James refers to, that have hard-coded -march options, really do emit 
armv4 code with Neon, say, although most probably don't have 
vectorizable code. They only work because they're most likely executed 
on Neon hardware.

This means that there's no obvious patch to fix the issue, in the 
compiler. It's easy to reject Neon for pre-v7 CPUs, but that has 
consequences, as we've seen. We'd have to have a table of fall-back FPUs 
or something, and that doesn't seem straight-forward (and anyway, I'm 
not sure what values to enter into that table).

So, I've attacked the problem from the other end, and updated the 
compiler check.

OK to commit?

Andrew

Comments

Andrew Stubbs Jan. 12, 2015, 1:16 p.m. UTC | #1
Ping.

On 23/12/14 16:46, Andrew Stubbs wrote:
> On 03/12/14 15:03, Andrew Stubbs wrote:
>>> 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.
>
> Or, not so shortly.
>
> It seems that, on ARM, the arch/CPU setting is basically orthogonal to
> the FPU setting, and the compiler doesn't even try to match the one to
> the other. The assembler does the same. In fact, the testcases that
> James refers to, that have hard-coded -march options, really do emit
> armv4 code with Neon, say, although most probably don't have
> vectorizable code. They only work because they're most likely executed
> on Neon hardware.
>
> This means that there's no obvious patch to fix the issue, in the
> compiler. It's easy to reject Neon for pre-v7 CPUs, but that has
> consequences, as we've seen. We'd have to have a table of fall-back FPUs
> or something, and that doesn't seem straight-forward (and anyway, I'm
> not sure what values to enter into that table).
>
> So, I've attacked the problem from the other end, and updated the
> compiler check.
>
> OK to commit?
>
> Andrew
Ramana Radhakrishnan Jan. 12, 2015, 1:50 p.m. UTC | #2
Sorry about the slow response- have been on holiday and still catching 
up on email.

On 12/01/15 13:16, Andrew Stubbs wrote:
> Ping.
>
> On 23/12/14 16:46, Andrew Stubbs wrote:
>> On 03/12/14 15:03, Andrew Stubbs wrote:
>>>> 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.
>>
>> Or, not so shortly.
>>

Sigh.


>> It seems that, on ARM, the arch/CPU setting is basically orthogonal to
>> the FPU setting, and the compiler doesn't even try to match the one to
>> the other. The assembler does the same. In fact, the testcases that
>> James refers to, that have hard-coded -march options, really do emit
>> armv4 code with Neon, say, although most probably don't have
>> vectorizable code. They only work because they're most likely executed
>> on Neon hardware.

Yes - though I'm surprised as I run an armv5te soft float only test run 
once a while on my Sheevaplug and don't see these issues. Maybe others do.

>>
>> This means that there's no obvious patch to fix the issue, in the
>> compiler. It's easy to reject Neon for pre-v7 CPUs, but that has
>> consequences, as we've seen. We'd have to have a table of fall-back FPUs
>> or something, and that doesn't seem straight-forward (and anyway, I'm
>> not sure what values to enter into that table).
>>
>> So, I've attacked the problem from the other end, and updated the
>> compiler check.
>>
>> OK to commit?

In principle ok, but I'd like a comment in there explaining why we've 
done this. Can you also post under what configurations these have been 
tested ?


Ramana

>>
>> Andrew
>
diff mbox

Patch

2014-12-23  Andrew Stubbs  <ams@codesourcery.com>

	gcc/testsuite/
	* lib/target-supports.exp
	(check_effective_target_arm_neon_ok_nocache): Don't try to test Neon
	on ARM architures before v7.

Index: gcc/testsuite/lib/target-supports.exp
===================================================================
--- gcc/testsuite/lib/target-supports.exp	(revision 219043)
+++ gcc/testsuite/lib/target-supports.exp	(working copy)
@@ -2565,6 +2565,9 @@ 
 	    if { [check_no_compiler_messages_nocache arm_neon_ok object {
 		#include "arm_neon.h"
 		int dummy;
+		#if __ARM_ARCH < 7
+		#error Architecture too old for NEON.
+		#endif
 	    } "$flags"] } {
 		set et_arm_neon_flags $flags
 		return 1