[patch, testsuite, arm] Fix ICE in gcc.dg/gimplefe-28.c

[patch, testsuite, arm] Fix ICE in gcc.dg/gimplefe-28.c

[Sandra Loosemore]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

For the default multilib on arm-none-eabi, gcc.dg/gimplefe-28 has been 
getting an ICE because, while the target-supports infrastructure is 
probing to see if it can add the command-line options to enable the sqrt 
insn ("-mfpu=vfp -mfloat-abi=softfp"), it is not actually adding those 
options when building this testcase.  :-S  The hook to do this is 
already there; it just needs a case for arm.

OK to commit?

-Sandra

[-- Attachment #2: arm-sqrt.log --]
[-- Type: text/x-log, Size: 197 bytes --]

2019-09-13  Sandra Loosemore  <sandra@codesourcery.com>

	gcc/testsuite/
	* lib/target-supports.exp (add_options_for_sqrt_insn): Add
	arm options consistent with check_effective_target_arm_vfp_ok.

[-- Attachment #3: arm-sqrt.patch --]
[-- Type: text/x-patch, Size: 479 bytes --]

Index: gcc/testsuite/lib/target-supports.exp
===================================================================
--- gcc/testsuite/lib/target-supports.exp	(revision 275699)
+++ gcc/testsuite/lib/target-supports.exp	(working copy)
@@ -6670,6 +6670,9 @@ proc add_options_for_sqrt_insn { flags }
     if { [istarget amdgcn*-*-*] } {
 	return "$flags -ffast-math"
     }
+    if { [istarget arm*-*-*] } {
+	return "$flags -mfpu=vfp -mfloat-abi=softfp"
+    }
     return $flags
 }
 

Re: [patch, testsuite, arm] Fix ICE in gcc.dg/gimplefe-28.c

[Mike Stump]

On Sep 13, 2019, at 12:06 PM, Sandra Loosemore <sandra@codesourcery.com> wrote:
> 
> For the default multilib on arm-none-eabi, gcc.dg/gimplefe-28 has been getting an ICE because, while the target-supports infrastructure is probing to see if it can add the command-line options to enable the sqrt insn ("-mfpu=vfp -mfloat-abi=softfp"), it is not actually adding those options when building this testcase.  :-S  The hook to do this is already there; it just needs a case for arm.
> 
> OK to commit?

Ok.

Hum, usually the arm people are so responsive.  I don't think I've seen a review of this, so when they don't, I will.

General note, I do prefer the target folk chime in on such patches instead of me, as there can be subtle target things that target folks track better than I and arm is one of those targets with so many wonder and subtle things.  :-)

Re: [patch, testsuite, arm] Fix ICE in gcc.dg/gimplefe-28.c

[Kyrill Tkachov]

Hi Sandra, Mike,

On 9/18/19 7:14 PM, Mike Stump wrote:
> On Sep 13, 2019, at 12:06 PM, Sandra Loosemore 
> <sandra@codesourcery.com> wrote:
> >
> > For the default multilib on arm-none-eabi, gcc.dg/gimplefe-28 has 
> been getting an ICE because, while the target-supports infrastructure 
> is probing to see if it can add the command-line options to enable the 
> sqrt insn ("-mfpu=vfp -mfloat-abi=softfp"), it is not actually adding 
> those options when building this testcase.  :-S  The hook to do this 
> is already there; it just needs a case for arm.
> >
> > OK to commit?
>
> Ok.
>
> Hum, usually the arm people are so responsive.  I don't think I've 
> seen a review of this, so when they don't, I will.
>
> General note, I do prefer the target folk chime in on such patches 
> instead of me, as there can be subtle target things that target folks 
> track better than I and arm is one of those targets with so many 
> wonder and subtle things.  :-)

I'm sorry for this. It slipped through the cracks among the big arm 
patch series recently (DImode cleanups and FDPIC).

In the future, putting the maintainers on CC in the submission helps, at 
least for me, to make it more visible.

As for the patch...


arm-sqrt.log

2019-09-13  Sandra Loosemore<sandra@codesourcery.com>

	gcc/testsuite/
	* lib/target-supports.exp (add_options_for_sqrt_insn): Add
	arm options consistent with check_effective_target_arm_vfp_ok.


arm-sqrt.patch

Index: gcc/testsuite/lib/target-supports.exp
===================================================================
--- gcc/testsuite/lib/target-supports.exp	(revision 275699)
+++ gcc/testsuite/lib/target-supports.exp	(working copy)
@@ -6670,6 +6670,9 @@ proc add_options_for_sqrt_insn { flags }
      if { [istarget amdgcn*-*-*] } {
  	return "$flags -ffast-math"
      }
+    if { [istarget arm*-*-*] } {
+	return "$flags -mfpu=vfp -mfloat-abi=softfp"
+    }
      return $flags
  }

I tend to think it's a bit cleaner to use one of the add_options_for_* helpers we have that know the right float-abi and mfpu options to pass.
Would using add_options_for_arm_fp or add_options_for_arm_vfp3 work here?

Thanks,
Kyrill

Re: [patch, testsuite, arm] Fix ICE in gcc.dg/gimplefe-28.c

[Sandra Loosemore]

On 9/19/19 2:40 AM, Kyrill Tkachov wrote:

> Index: gcc/testsuite/lib/target-supports.exp
> ===================================================================
> --- gcc/testsuite/lib/target-supports.exp    (revision 275699)
> +++ gcc/testsuite/lib/target-supports.exp    (working copy)
> @@ -6670,6 +6670,9 @@ proc add_options_for_sqrt_insn { flags }
>       if { [istarget amdgcn*-*-*] } {
>       return "$flags -ffast-math"
>       }
> +    if { [istarget arm*-*-*] } {
> +    return "$flags -mfpu=vfp -mfloat-abi=softfp"
> +    }
>       return $flags
>   }
> 
> I tend to think it's a bit cleaner to use one of the add_options_for_* 
> helpers we have that know the right float-abi and mfpu options to pass.
> Would using add_options_for_arm_fp or add_options_for_arm_vfp3 work here?

add_options_for_arm_fp only adds a -mfloat-abi option which isn't 
sufficient by itself to enable floating-point on the default multilib 
for arm-none-eabi.  It needs -mfpu= too.

My understanding is that all VFP fpus have support for sqrt so why 
require vfp3?

I could put the magic options in a new function called 
add_options_for_arm_vfp instead of directly in 
add_options_for_sqrt_insn, and fix it up to cope with -mfloat-abi=hard 
multilibs too.  Would that be an improvement?

-Sandra

Re: [patch, testsuite, arm] Fix ICE in gcc.dg/gimplefe-28.c

[Kyrill Tkachov]

On 9/19/19 7:44 PM, Sandra Loosemore wrote:
> On 9/19/19 2:40 AM, Kyrill Tkachov wrote:
>
>> Index: gcc/testsuite/lib/target-supports.exp
>> ===================================================================
>> --- gcc/testsuite/lib/target-supports.exp    (revision 275699)
>> +++ gcc/testsuite/lib/target-supports.exp    (working copy)
>> @@ -6670,6 +6670,9 @@ proc add_options_for_sqrt_insn { flags }
>>       if { [istarget amdgcn*-*-*] } {
>>       return "$flags -ffast-math"
>>       }
>> +    if { [istarget arm*-*-*] } {
>> +    return "$flags -mfpu=vfp -mfloat-abi=softfp"
>> +    }
>>       return $flags
>>   }
>>
>> I tend to think it's a bit cleaner to use one of the 
>> add_options_for_* helpers we have that know the right float-abi and 
>> mfpu options to pass.
>> Would using add_options_for_arm_fp or add_options_for_arm_vfp3 work 
>> here?
>
> add_options_for_arm_fp only adds a -mfloat-abi option which isn't 
> sufficient by itself to enable floating-point on the default multilib 
> for arm-none-eabi.  It needs -mfpu= too.
>
> My understanding is that all VFP fpus have support for sqrt so why 
> require vfp3?
>
> I could put the magic options in a new function called 
> add_options_for_arm_vfp instead of directly in 
> add_options_for_sqrt_insn, and fix it up to cope with -mfloat-abi=hard 
> multilibs too.  Would that be an improvement?

Yeah, an add_options_for_arm_vfp is what we ideally need here.

Thanks,

Kyrill



>
> -Sandra

Re: [patch, testsuite, arm] Fix ICE in gcc.dg/gimplefe-28.c

[Sandra Loosemore]

[-- Attachment #1: Type: text/plain, Size: 278 bytes --]

On 9/20/19 2:18 AM, Kyrill Tkachov wrote:
> 
> Yeah, an add_options_for_arm_vfp is what we ideally need here.

How about this version of the patch?  The two test cases I also tweaked 
to use it are the only ones that use the corresponding arm_vfp_ok 
effective target.

-Sandra

[-- Attachment #2: arm-sqrt2.log --]
[-- Type: text/x-log, Size: 390 bytes --]

2019-09-22  Sandra Loosemore  <sandra@codesourcery.com>

	gcc/testsuite/
	* lib/target-supports.exp
	(check_effective_target_arm_vfp_ok_nocache): New.
	(check_effective_target_arm_vfp_ok): Rewrite.
	(add_options_for_arm_vfp): New.
	(add_options_for_sqrt_insn): Add options for arm.
	* gcc.target/arm/attr-neon-builtin-fail2.c: Use dg-add-options.
	* gcc.target/arm/short-vfp-1.c: Likewise.

[-- Attachment #3: arm-sqrt2.patch --]
[-- Type: text/x-patch, Size: 3051 bytes --]

Index: gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c
===================================================================
--- gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c	(revision 276004)
+++ gcc/testsuite/gcc.target/arm/attr-neon-builtin-fail2.c	(working copy)
@@ -1,7 +1,8 @@
 /* Check that calling a neon builtin from a function compiled with vfp fails.  */
 /* { dg-do compile } */
 /* { dg-require-effective-target arm_vfp_ok } */
-/* { dg-options "-O2 -mfloat-abi=softfp" } */
+/* { dg-options "-O2" } */
+/* { dg-add-options arm_vfp } */
 
 extern __simd64_int8_t a, b;
 
Index: gcc/testsuite/gcc.target/arm/short-vfp-1.c
===================================================================
--- gcc/testsuite/gcc.target/arm/short-vfp-1.c	(revision 276004)
+++ gcc/testsuite/gcc.target/arm/short-vfp-1.c	(working copy)
@@ -1,6 +1,6 @@
 /* { dg-do compile } */
 /* { dg-require-effective-target arm_vfp_ok }
-/* { dg-options "-mfpu=vfp" } */
+/* { dg-add-options arm_vfp } */
 
 int
 test_sisf (float x)
Index: gcc/testsuite/lib/target-supports.exp
===================================================================
--- gcc/testsuite/lib/target-supports.exp	(revision 276004)
+++ gcc/testsuite/lib/target-supports.exp	(working copy)
@@ -3489,18 +3489,43 @@ proc check_effective_target_arm_soft_ok
     }
 }
 
-# Return 1 if this is an ARM target supporting -mfpu=vfp
-# -mfloat-abi=softfp.  Some multilibs may be incompatible with these
-# options.
+# Return 1 if this is an ARM target supporting -mfpu=vfp with an
+# appropriate abi.
 
-proc check_effective_target_arm_vfp_ok { } {
+proc check_effective_target_arm_vfp_ok_nocache { } {
+    global et_arm_vfp_flags
+    set et_arm_vfp_flags ""
     if { [check_effective_target_arm32] } {
-	return [check_no_compiler_messages arm_vfp_ok object {
-	    int dummy;
-	} "-mfpu=vfp -mfloat-abi=softfp"]
-    } else {
-	return 0
+	foreach flags {"-mfpu=vfp" "-mfpu=vfp -mfloat-abi=softfp" "-mpu=vfp -mfloat-abi=hard"} {
+	    if { [check_no_compiler_messages_nocache arm_fp_ok object {
+		#ifndef __ARM_FP
+		#error __ARM_FP not defined
+		#endif
+	    } "$flags"] } {
+		set et_arm_vfp_flags $flags
+		return 1
+	    }
+	}
     }
+
+    return 0
+}
+
+proc check_effective_target_arm_vfp_ok { } {
+    return [check_cached_effective_target arm_vfp_ok \
+		check_effective_target_arm_vfp_ok_nocache]
+}
+
+# Add the options needed to compile code with -mfpu=vfp.  We need either
+# -mfloat-abi=softfp or -mfloat-abi=hard, but if one is already
+# specified by the multilib, use it.
+
+proc add_options_for_arm_vfp { flags } {
+    if { ! [check_effective_target_arm_vfp_ok] } {
+	return "$flags"
+    }
+    global et_arm_vfp_flags
+    return "$flags $et_arm_vfp_flags"
 }
 
 # Return 1 if this is an ARM target supporting -mfpu=vfp3
@@ -6678,6 +6703,9 @@ proc add_options_for_sqrt_insn { flags }
     if { [istarget amdgcn*-*-*] } {
 	return "$flags -ffast-math"
     }
+    if { [istarget arm*-*-*] } {
+	return [add_options_for_arm_vfp "$flags"]
+    }
     return $flags
 }
 

Re: [patch, testsuite, arm] Fix ICE in gcc.dg/gimplefe-28.c

[Kyrill Tkachov]

On 9/23/19 4:11 AM, Sandra Loosemore wrote:
> On 9/20/19 2:18 AM, Kyrill Tkachov wrote:
>>
>> Yeah, an add_options_for_arm_vfp is what we ideally need here.
>
> How about this version of the patch?  The two test cases I also 
> tweaked to use it are the only ones that use the corresponding 
> arm_vfp_ok effective target.
>
That looks good.

Ok.

Thanks,

Kyrill



> -Sandra