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

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

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.  :-)

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

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

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

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

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