[Arm] Add HF modes to ANY iterators

Hi All,

The iterators ANY64 and ANY128 are used in various general split patterns and
are supposed to contain any 64 bit and 128 bit modes respectively.

For some reason these patterns had HI but not HF.  This adds HF so that general
64 and 128 bit splits are generated for these modes as well.  These are required
by various split patterns that expect them to be there.

Bootstrapped Regtested on aarch64-none-linux-gnu and <native regtest still running> issues.

Ok for trunk?

Thanks,
Tamar

gcc/ChangeLog:

2019-02-13  Tamar Christina  <tamar.christina@arm.com>

	PR target/88850
	* config/arm/iterators.md (ANY64): Add V4HF,
	(ANY128): Add V8HF.

gcc/testsuite/ChangeLog:

2019-02-13  Tamar Christina  <tamar.christina@arm.com>

	PR target/88850
	* gcc.target/arm/pr88850-2.c: New test.

--

Hi Tamar

On 2/13/19 10:33 AM, Tamar Christina wrote:
> Hi All,
>
> The iterators ANY64 and ANY128 are used in various general split 
> patterns and
> are supposed to contain any 64 bit and 128 bit modes respectively.
>
> For some reason these patterns had HI but not HF.  This adds HF so 
> that general
> 64 and 128 bit splits are generated for these modes as well.  These 
> are required
> by various split patterns that expect them to be there.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu and <native regtest 
> still running> issues.
>
Please do this on an arm-none-linux-gnueabihf target.

Though I suspect this is just a placeholder from a boilerplate ;)

> Ok for trunk?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
>
>         PR target/88850
>         * config/arm/iterators.md (ANY64): Add V4HF,
>         (ANY128): Add V8HF.
>
> gcc/testsuite/ChangeLog:
>
> 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
>
>         PR target/88850
>         * gcc.target/arm/pr88850-2.c: New test.
>
> -- 

diff --git a/gcc/config/arm/iterators.md b/gcc/config/arm/iterators.md
index c33e572c3e89c3dc5848bd6b825d618481247558..4ac048a0c609273691c264c97ccf6cd47b43943b 100644
--- a/gcc/config/arm/iterators.md
+++ b/gcc/config/arm/iterators.md
@@ -24,11 +24,11 @@
  ;;----------------------------------------------------------------------------
  
  ;; A list of modes that are exactly 64 bits in size. This is used to expand
-;; some splits that are the same for all modes when operating on ARM
+;; some splits that are the same for all modes when operating on ARM
  ;; registers.
-(define_mode_iterator ANY64 [DI DF V8QI V4HI V2SI V2SF])
+(define_mode_iterator ANY64 [DI DF V8QI V4HI V4HF V2SI V2SF])
  
-(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V4SI V4SF])
+(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V8HF V4SI V4SF])
  

I see ANY128 is used in the move_hi_quad_<mode> and move_lo_quad_<mode> expanders in neon.md.
Does the use of HF modes need guarding by TARGET_NEON_FP16INST there?
Would be good to see a test exercising this change...


  ;; A list of integer modes that are up to one word long
  (define_mode_iterator QHSI [QI HI SI])
diff --git a/gcc/testsuite/gcc.target/arm/pr88850-2.c b/gcc/testsuite/gcc.target/arm/pr88850-2.c
new file mode 100644
index 0000000000000000000000000000000000000000..fee2a0fe3a227ea4652bea2aa0e7335d20a7e9b5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr88850-2.c
@@ -0,0 +1,19 @@
+/* PR target/88850.  */
+/* { dg-do compile } */
+/* { dg-additional-options "-O2 -march=armv7-a -mfloat-abi=softfp -fdump-rtl-final" } */
+/* { dg-add-options arm_neon_fp16 } */
+/* { dg-require-effective-target arm_soft_ok } */
+/* { dg-require-effective-target arm_neon_fp16_ok } */
+

arm_soft_ok checks if it's okay to add -mfloat-abi=soft, but you're adding softfp.
Is this right?

Thanks,
Kyrill

+#include <arm_neon.h>
+
+extern void c (int, float16x4_t);
+
+void a (float16x4_t b)
+{
+  c (0, b);
+}
+
+
+/* Check that these 64-bit moves are done in SI.  */
+/* { dg-final { scan-rtl-dump "_movsi_vfp" "final" } } *

The 02/13/2019 10:57, Kyrill Tkachov wrote:
> Hi Tamar
> 
> On 2/13/19 10:33 AM, Tamar Christina wrote:
> > Hi All,
> >
> > The iterators ANY64 and ANY128 are used in various general split 
> > patterns and
> > are supposed to contain any 64 bit and 128 bit modes respectively.
> >
> > For some reason these patterns had HI but not HF.  This adds HF so 
> > that general
> > 64 and 128 bit splits are generated for these modes as well.  These 
> > are required
> > by various split patterns that expect them to be there.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and <native regtest 
> > still running> issues.
> >
> Please do this on an arm-none-linux-gnueabihf target.
> 
> Though I suspect this is just a placeholder from a boilerplate ;)

Oops, yes sorry, used the wrong template.. and forgot to fill in the results.. oh my.

Bootstrapped and regtested on arm-none-linux-gnueabihf and no issues :)

> 
> > Ok for trunk?
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
> >
> >         PR target/88850
> >         * config/arm/iterators.md (ANY64): Add V4HF,
> >         (ANY128): Add V8HF.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
> >
> >         PR target/88850
> >         * gcc.target/arm/pr88850-2.c: New test.
> >
> > -- 
> 
> diff --git a/gcc/config/arm/iterators.md b/gcc/config/arm/iterators.md
> index c33e572c3e89c3dc5848bd6b825d618481247558..4ac048a0c609273691c264c97ccf6cd47b43943b 100644
> --- a/gcc/config/arm/iterators.md
> +++ b/gcc/config/arm/iterators.md
> @@ -24,11 +24,11 @@
>   ;;----------------------------------------------------------------------------
>   
>   ;; A list of modes that are exactly 64 bits in size. This is used to expand
> -;; some splits that are the same for all modes when operating on ARM
> +;; some splits that are the same for all modes when operating on ARM
>   ;; registers.
> -(define_mode_iterator ANY64 [DI DF V8QI V4HI V2SI V2SF])
> +(define_mode_iterator ANY64 [DI DF V8QI V4HI V4HF V2SI V2SF])
>   
> -(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V4SI V4SF])
> +(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V8HF V4SI V4SF])
>   
> 
> I see ANY128 is used in the move_hi_quad_<mode> and move_lo_quad_<mode> expanders in neon.md.
> Does the use of HF modes need guarding by TARGET_NEON_FP16INST there?

I don't think it does, the patterns would generate a V_HALF move which should match either
mov<mode> with the VH iterator or *neon_mov<mode> with VQXMOV, both of which already
contain V4HF. This is because I think it's just going to move them around in s registers without caring what's
in them as the bits are not being manipulated so you don't need fp16 instructions.

> Would be good to see a test exercising this change...

Would you be able to guide me in the direction of how? It seems practically impossible to generate this pattern from C.
The move_hi_quad and move_lo_quad are inserts, but using neon intrinsics like vcombine_f16 doesn't generate this RTL pattern.
The only thing that uses these things is vec_pack_trunc_  and the only thing that seems to use this is the auto vectorizer.

The autovectorizer won't ever generate a V8HF case because my previous patch to enable autovectorization of vector HFmodes was
deferred to GCC 10 as the patch series was cannibalized.

I'm more than happy not to change ANY128 if you prefer.

Thanks,
Tamar

> 
> 
>   ;; A list of integer modes that are up to one word long
>   (define_mode_iterator QHSI [QI HI SI])
> diff --git a/gcc/testsuite/gcc.target/arm/pr88850-2.c b/gcc/testsuite/gcc.target/arm/pr88850-2.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..fee2a0fe3a227ea4652bea2aa0e7335d20a7e9b5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr88850-2.c
> @@ -0,0 +1,19 @@
> +/* PR target/88850.  */
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O2 -march=armv7-a -mfloat-abi=softfp -fdump-rtl-final" } */
> +/* { dg-add-options arm_neon_fp16 } */
> +/* { dg-require-effective-target arm_soft_ok } */
> +/* { dg-require-effective-target arm_neon_fp16_ok } */
> +
> 
> arm_soft_ok checks if it's okay to add -mfloat-abi=soft, but you're adding softfp.
> Is this right?
> 
> Thanks,
> Kyrill
> 
> +#include <arm_neon.h>
> +
> +extern void c (int, float16x4_t);
> +
> +void a (float16x4_t b)
> +{
> +  c (0, b);
> +}
> +
> +
> +/* Check that these 64-bit moves are done in SI.  */
> +/* { dg-final { scan-rtl-dump "_movsi_vfp" "final" } } *
> 

--

Hi Tamar,

On 2/13/19 4:31 PM, Tamar Christina wrote:
> The 02/13/2019 10:57, Kyrill Tkachov wrote:
>> Hi Tamar
>>
>> On 2/13/19 10:33 AM, Tamar Christina wrote:
>>> Hi All,
>>>
>>> The iterators ANY64 and ANY128 are used in various general split
>>> patterns and
>>> are supposed to contain any 64 bit and 128 bit modes respectively.
>>>
>>> For some reason these patterns had HI but not HF.  This adds HF so
>>> that general
>>> 64 and 128 bit splits are generated for these modes as well.  These
>>> are required
>>> by various split patterns that expect them to be there.
>>>
>>> Bootstrapped Regtested on aarch64-none-linux-gnu and <native regtest
>>> still running> issues.
>>>
>> Please do this on an arm-none-linux-gnueabihf target.
>>
>> Though I suspect this is just a placeholder from a boilerplate ;)
> Oops, yes sorry, used the wrong template.. and forgot to fill in the results.. oh my.
>
> Bootstrapped and regtested on arm-none-linux-gnueabihf and no issues :)
>
>>> Ok for trunk?
>>>
>>> Thanks,
>>> Tamar
>>>
>>> gcc/ChangeLog:
>>>
>>> 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
>>>
>>>          PR target/88850
>>>          * config/arm/iterators.md (ANY64): Add V4HF,
>>>          (ANY128): Add V8HF.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
>>>
>>>          PR target/88850
>>>          * gcc.target/arm/pr88850-2.c: New test.
>>>
>>> -- 
>> diff --git a/gcc/config/arm/iterators.md b/gcc/config/arm/iterators.md
>> index c33e572c3e89c3dc5848bd6b825d618481247558..4ac048a0c609273691c264c97ccf6cd47b43943b 100644
>> --- a/gcc/config/arm/iterators.md
>> +++ b/gcc/config/arm/iterators.md
>> @@ -24,11 +24,11 @@
>>    ;;----------------------------------------------------------------------------
>>    
>>    ;; A list of modes that are exactly 64 bits in size. This is used to expand
>> -;; some splits that are the same for all modes when operating on ARM
>> +;; some splits that are the same for all modes when operating on ARM
>>    ;; registers.
>> -(define_mode_iterator ANY64 [DI DF V8QI V4HI V2SI V2SF])
>> +(define_mode_iterator ANY64 [DI DF V8QI V4HI V4HF V2SI V2SF])
>>    
>> -(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V4SI V4SF])
>> +(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V8HF V4SI V4SF])
>>    
>>
>> I see ANY128 is used in the move_hi_quad_<mode> and move_lo_quad_<mode> expanders in neon.md.
>> Does the use of HF modes need guarding by TARGET_NEON_FP16INST there?
> I don't think it does, the patterns would generate a V_HALF move which should match either
> mov<mode> with the VH iterator or *neon_mov<mode> with VQXMOV, both of which already
> contain V4HF. This is because I think it's just going to move them around in s registers without caring what's
> in them as the bits are not being manipulated so you don't need fp16 instructions.
>
>> Would be good to see a test exercising this change...
> Would you be able to guide me in the direction of how? It seems practically impossible to generate this pattern from C.
> The move_hi_quad and move_lo_quad are inserts, but using neon intrinsics like vcombine_f16 doesn't generate this RTL pattern.
> The only thing that uses these things is vec_pack_trunc_  and the only thing that seems to use this is the auto vectorizer.
>
> The autovectorizer won't ever generate a V8HF case because my previous patch to enable autovectorization of vector HFmodes was
> deferred to GCC 10 as the patch series was cannibalized.
>
> I'm more than happy not to change ANY128 if you prefer.

Yeah, that's what I was getting at eventually :)

Let's leave ANY128 alone for now.

Kyrill

> Thanks,
> Tamar
>
>>
>>    ;; A list of integer modes that are up to one word long
>>    (define_mode_iterator QHSI [QI HI SI])
>> diff --git a/gcc/testsuite/gcc.target/arm/pr88850-2.c b/gcc/testsuite/gcc.target/arm/pr88850-2.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..fee2a0fe3a227ea4652bea2aa0e7335d20a7e9b5
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/pr88850-2.c
>> @@ -0,0 +1,19 @@
>> +/* PR target/88850.  */
>> +/* { dg-do compile } */
>> +/* { dg-additional-options "-O2 -march=armv7-a -mfloat-abi=softfp -fdump-rtl-final" } */
>> +/* { dg-add-options arm_neon_fp16 } */
>> +/* { dg-require-effective-target arm_soft_ok } */
>> +/* { dg-require-effective-target arm_neon_fp16_ok } */
>> +
>>
>> arm_soft_ok checks if it's okay to add -mfloat-abi=soft, but you're adding softfp.
>> Is this right?
>>
>> Thanks,
>> Kyrill
>>
>> +#include <arm_neon.h>
>> +
>> +extern void c (int, float16x4_t);
>> +
>> +void a (float16x4_t b)
>> +{
>> +  c (0, b);
>> +}
>> +
>> +
>> +/* Check that these 64-bit moves are done in SI.  */
>> +/* { dg-final { scan-rtl-dump "_movsi_vfp" "final" } } *
>>

Hi Kyrill,

I couldn't find a way to actually generate this case so I have instead removed
the entry from ANY128.  New patch and changelog below.

--

The iterator ANY64 are used in various general split patterns and is supposed
to contain all 64 bit modes.

For some reason the pattern has HI but not HF.  This adds HF so that general
64 bit splits are generated for these modes as well.  These are required
by various split patterns that expect them to be there.

Bootstrapped Regtested on arm-none-gnueabihf and no issues.

Ok for trunk?

Thanks,
Tamar

gcc/ChangeLog:

2019-02-14  Tamar Christina  <tamar.christina@arm.com>

	PR target/88850
	* config/arm/iterators.md (ANY64): Add V4HF.

gcc/testsuite/ChangeLog:

2019-02-14  Tamar Christina  <tamar.christina@arm.com>

	PR target/88850
	* gcc.target/arm/pr88850-2.c: New test.
	* lib/target-supports.exp
	(check_effective_target_arm_neon_softfp_fp16_ok_nocache,
	check_effective_target_arm_neon_softfp_fp16_ok,
	add_options_for_arm_neon_softfp_fp16): New.


The 02/13/2019 10:57, Kyrill Tkachov wrote:
> Hi Tamar
> 
> On 2/13/19 10:33 AM, Tamar Christina wrote:
> > Hi All,
> >
> > The iterators ANY64 and ANY128 are used in various general split 
> > patterns and
> > are supposed to contain any 64 bit and 128 bit modes respectively.
> >
> > For some reason these patterns had HI but not HF.  This adds HF so 
> > that general
> > 64 and 128 bit splits are generated for these modes as well.  These 
> > are required
> > by various split patterns that expect them to be there.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and <native regtest 
> > still running> issues.
> >
> Please do this on an arm-none-linux-gnueabihf target.
> 
> Though I suspect this is just a placeholder from a boilerplate ;)
> 
> > Ok for trunk?
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
> >
> >         PR target/88850
> >         * config/arm/iterators.md (ANY64): Add V4HF,
> >         (ANY128): Add V8HF.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
> >
> >         PR target/88850
> >         * gcc.target/arm/pr88850-2.c: New test.
> >
> > -- 
> 
> diff --git a/gcc/config/arm/iterators.md b/gcc/config/arm/iterators.md
> index c33e572c3e89c3dc5848bd6b825d618481247558..4ac048a0c609273691c264c97ccf6cd47b43943b 100644
> --- a/gcc/config/arm/iterators.md
> +++ b/gcc/config/arm/iterators.md
> @@ -24,11 +24,11 @@
>   ;;----------------------------------------------------------------------------
>   
>   ;; A list of modes that are exactly 64 bits in size. This is used to expand
> -;; some splits that are the same for all modes when operating on ARM
> +;; some splits that are the same for all modes when operating on ARM
>   ;; registers.
> -(define_mode_iterator ANY64 [DI DF V8QI V4HI V2SI V2SF])
> +(define_mode_iterator ANY64 [DI DF V8QI V4HI V4HF V2SI V2SF])
>   
> -(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V4SI V4SF])
> +(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V8HF V4SI V4SF])
>   
> 
> I see ANY128 is used in the move_hi_quad_<mode> and move_lo_quad_<mode> expanders in neon.md.
> Does the use of HF modes need guarding by TARGET_NEON_FP16INST there?
> Would be good to see a test exercising this change...
> 
> 
>   ;; A list of integer modes that are up to one word long
>   (define_mode_iterator QHSI [QI HI SI])
> diff --git a/gcc/testsuite/gcc.target/arm/pr88850-2.c b/gcc/testsuite/gcc.target/arm/pr88850-2.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..fee2a0fe3a227ea4652bea2aa0e7335d20a7e9b5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr88850-2.c
> @@ -0,0 +1,19 @@
> +/* PR target/88850.  */
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O2 -march=armv7-a -mfloat-abi=softfp -fdump-rtl-final" } */
> +/* { dg-add-options arm_neon_fp16 } */
> +/* { dg-require-effective-target arm_soft_ok } */
> +/* { dg-require-effective-target arm_neon_fp16_ok } */
> +
> 
> arm_soft_ok checks if it's okay to add -mfloat-abi=soft, but you're adding softfp.
> Is this right?
> 
> Thanks,
> Kyrill
> 
> +#include <arm_neon.h>
> +
> +extern void c (int, float16x4_t);
> +
> +void a (float16x4_t b)
> +{
> +  c (0, b);
> +}
> +
> +
> +/* Check that these 64-bit moves are done in SI.  */
> +/* { dg-final { scan-rtl-dump "_movsi_vfp" "final" } } *
> 

--
diff --git a/gcc/config/arm/iterators.md b/gcc/config/arm/iterators.md
index c33e572c3e89c3dc5848bd6b825d618481247558..eb07c5b90c1b1905d35d7b480bdbe7d7a45ab7ba 100644
--- a/gcc/config/arm/iterators.md
+++ b/gcc/config/arm/iterators.md
@@ -24,9 +24,9 @@
 ;;----------------------------------------------------------------------------
 
 ;; A list of modes that are exactly 64 bits in size. This is used to expand
-;; some splits that are the same for all modes when operating on ARM 
+;; some splits that are the same for all modes when operating on ARM
 ;; registers.
-(define_mode_iterator ANY64 [DI DF V8QI V4HI V2SI V2SF])
+(define_mode_iterator ANY64 [DI DF V8QI V4HI V4HF V2SI V2SF])
 
 (define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V4SI V4SF])
 
diff --git a/gcc/testsuite/gcc.target/arm/pr88850-2.c b/gcc/testsuite/gcc.target/arm/pr88850-2.c
new file mode 100644
index 0000000000000000000000000000000000000000..7a1aec55dc70625bd6306e8d6bf094e11afe81bc
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr88850-2.c
@@ -0,0 +1,18 @@
+/* PR target/88850.  */
+/* { dg-do compile } */
+/* { dg-additional-options "-O2 -march=armv7-a -fdump-rtl-final" } */
+/* { dg-add-options arm_neon_softfp_fp16 } */
+/* { dg-require-effective-target arm_neon_softfp_fp16_ok } */
+
+#include <arm_neon.h>
+
+extern void c (int, float16x4_t);
+
+void a (float16x4_t b)
+{
+  c (0, b);
+}
+
+
+/* Check that these 64-bit moves are done in SI.  */
+/* { dg-final { scan-rtl-dump "_movsi_vfp" "final" } } */
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index a0b4b99067f9ae225bde3b6bc719e89e1ea8e0e1..1d237d4cd664924cc580cff67a563230b3fe9571 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -3785,6 +3785,44 @@ proc check_effective_target_arm_neon_fp16_ok { } {
 		check_effective_target_arm_neon_fp16_ok_nocache]
 }
 
+# Return 1 if this is an ARM target supporting -mfpu=neon-fp16
+# and -mfloat-abi=softfp together.  Some multilibs may be
+# incompatible with these options.  Also set et_arm_neon_softfp_fp16_flags to
+# the best options to add.
+
+proc check_effective_target_arm_neon_softfp_fp16_ok_nocache { } {
+    global et_arm_neon_softfp_fp16_flags
+    global et_arm_neon_flags
+    set et_arm_neon_softfp_fp16_flags ""
+    if { [check_effective_target_arm32]
+	 && [check_effective_target_arm_neon_ok] } {
+	foreach flags {"-mfpu=neon-fp16 -mfloat-abi=softfp"
+		       "-mfloat-abi=softfp -mfp16-format=ieee"
+		       "-mfpu=neon-fp16 -mfloat-abi=softfp -mfp16-format=ieee"} {
+	    if { [check_no_compiler_messages_nocache arm_neon_softfp_fp16_ok object {
+		#include "arm_neon.h"
+		float16x4_t
+		foo (float32x4_t arg)
+		{
+                  return vcvt_f16_f32 (arg);
+		}
+	    } "$et_arm_neon_flags $flags"] } {
+		set et_arm_neon_softfp_fp16_flags [concat $et_arm_neon_flags $flags]
+		return 1
+	    }
+	}
+    }
+
+    return 0
+}
+
+proc check_effective_target_arm_neon_softfp_fp16_ok { } {
+    return [check_cached_effective_target arm_neon_softfp_fp16_ok \
+		check_effective_target_arm_neon_softfp_fp16_ok_nocache]
+}
+
+
+
 proc check_effective_target_arm_neon_fp16_hw { } {
     if {! [check_effective_target_arm_neon_fp16_ok] } {
 	return 0
@@ -3808,6 +3846,14 @@ proc add_options_for_arm_neon_fp16 { flags } {
     return "$flags $et_arm_neon_fp16_flags"
 }
 
+proc add_options_for_arm_neon_softfp_fp16 { flags } {
+    if { ! [check_effective_target_arm_neon_softfp_fp16_ok] } {
+	return "$flags"
+    }
+    global et_arm_neon_softfp_fp16_flags
+    return "$flags $et_arm_neon_softfp_fp16_flags"
+}
+
 # Return 1 if this is an ARM target supporting the FP16 alternative
 # format.  Some multilibs may be incompatible with the options needed.  Also
 # set et_arm_neon_fp16_flags to the best options to add.

On 2/14/19 4:52 PM, Tamar Christina wrote:
> Hi Kyrill,
>
> I couldn't find a way to actually generate this case so I have instead removed
> the entry from ANY128.  New patch and changelog below.
>
> --
>
> The iterator ANY64 are used in various general split patterns and is supposed
> to contain all 64 bit modes.
>
> For some reason the pattern has HI but not HF.  This adds HF so that general
> 64 bit splits are generated for these modes as well.  These are required
> by various split patterns that expect them to be there.
>
> Bootstrapped Regtested on arm-none-gnueabihf and no issues.
>
> Ok for trunk?

Ok.

Thanks,

Kyrill


> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> 2019-02-14  Tamar Christina  <tamar.christina@arm.com>
>
> 	PR target/88850
> 	* config/arm/iterators.md (ANY64): Add V4HF.
>
> gcc/testsuite/ChangeLog:
>
> 2019-02-14  Tamar Christina  <tamar.christina@arm.com>
>
> 	PR target/88850
> 	* gcc.target/arm/pr88850-2.c: New test.
> 	* lib/target-supports.exp
> 	(check_effective_target_arm_neon_softfp_fp16_ok_nocache,
> 	check_effective_target_arm_neon_softfp_fp16_ok,
> 	add_options_for_arm_neon_softfp_fp16): New.
>
>
> The 02/13/2019 10:57, Kyrill Tkachov wrote:
>> Hi Tamar
>>
>> On 2/13/19 10:33 AM, Tamar Christina wrote:
>>> Hi All,
>>>
>>> The iterators ANY64 and ANY128 are used in various general split
>>> patterns and
>>> are supposed to contain any 64 bit and 128 bit modes respectively.
>>>
>>> For some reason these patterns had HI but not HF.  This adds HF so
>>> that general
>>> 64 and 128 bit splits are generated for these modes as well.  These
>>> are required
>>> by various split patterns that expect them to be there.
>>>
>>> Bootstrapped Regtested on aarch64-none-linux-gnu and <native regtest
>>> still running> issues.
>>>
>> Please do this on an arm-none-linux-gnueabihf target.
>>
>> Though I suspect this is just a placeholder from a boilerplate ;)
>>
>>> Ok for trunk?
>>>
>>> Thanks,
>>> Tamar
>>>
>>> gcc/ChangeLog:
>>>
>>> 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
>>>
>>>          PR target/88850
>>>          * config/arm/iterators.md (ANY64): Add V4HF,
>>>          (ANY128): Add V8HF.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
>>>
>>>          PR target/88850
>>>          * gcc.target/arm/pr88850-2.c: New test.
>>>
>>> -- 
>> diff --git a/gcc/config/arm/iterators.md b/gcc/config/arm/iterators.md
>> index c33e572c3e89c3dc5848bd6b825d618481247558..4ac048a0c609273691c264c97ccf6cd47b43943b 100644
>> --- a/gcc/config/arm/iterators.md
>> +++ b/gcc/config/arm/iterators.md
>> @@ -24,11 +24,11 @@
>>    ;;----------------------------------------------------------------------------
>>    
>>    ;; A list of modes that are exactly 64 bits in size. This is used to expand
>> -;; some splits that are the same for all modes when operating on ARM
>> +;; some splits that are the same for all modes when operating on ARM
>>    ;; registers.
>> -(define_mode_iterator ANY64 [DI DF V8QI V4HI V2SI V2SF])
>> +(define_mode_iterator ANY64 [DI DF V8QI V4HI V4HF V2SI V2SF])
>>    
>> -(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V4SI V4SF])
>> +(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V8HF V4SI V4SF])
>>    
>>
>> I see ANY128 is used in the move_hi_quad_<mode> and move_lo_quad_<mode> expanders in neon.md.
>> Does the use of HF modes need guarding by TARGET_NEON_FP16INST there?
>> Would be good to see a test exercising this change...
>>
>>
>>    ;; A list of integer modes that are up to one word long
>>    (define_mode_iterator QHSI [QI HI SI])
>> diff --git a/gcc/testsuite/gcc.target/arm/pr88850-2.c b/gcc/testsuite/gcc.target/arm/pr88850-2.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..fee2a0fe3a227ea4652bea2aa0e7335d20a7e9b5
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/pr88850-2.c
>> @@ -0,0 +1,19 @@
>> +/* PR target/88850.  */
>> +/* { dg-do compile } */
>> +/* { dg-additional-options "-O2 -march=armv7-a -mfloat-abi=softfp -fdump-rtl-final" } */
>> +/* { dg-add-options arm_neon_fp16 } */
>> +/* { dg-require-effective-target arm_soft_ok } */
>> +/* { dg-require-effective-target arm_neon_fp16_ok } */
>> +
>>
>> arm_soft_ok checks if it's okay to add -mfloat-abi=soft, but you're adding softfp.
>> Is this right?
>>
>> Thanks,
>> Kyrill
>>
>> +#include <arm_neon.h>
>> +
>> +extern void c (int, float16x4_t);
>> +
>> +void a (float16x4_t b)
>> +{
>> +  c (0, b);
>> +}
>> +
>> +
>> +/* Check that these 64-bit moves are done in SI.  */
>> +/* { dg-final { scan-rtl-dump "_movsi_vfp" "final" } } *
>>

On Thu, 14 Feb 2019 at 17:52, Tamar Christina <Tamar.Christina@arm.com> wrote:
>
> Hi Kyrill,
>
> I couldn't find a way to actually generate this case so I have instead removed
> the entry from ANY128.  New patch and changelog below.
>
> --
>
> The iterator ANY64 are used in various general split patterns and is supposed
> to contain all 64 bit modes.
>
> For some reason the pattern has HI but not HF.  This adds HF so that general
> 64 bit splits are generated for these modes as well.  These are required
> by various split patterns that expect them to be there.
>
> Bootstrapped Regtested on arm-none-gnueabihf and no issues.
>
> Ok for trunk?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> 2019-02-14  Tamar Christina  <tamar.christina@arm.com>
>
>         PR target/88850
>         * config/arm/iterators.md (ANY64): Add V4HF.
>
> gcc/testsuite/ChangeLog:
>
> 2019-02-14  Tamar Christina  <tamar.christina@arm.com>
>
>         PR target/88850
>         * gcc.target/arm/pr88850-2.c: New test.
>         * lib/target-supports.exp
>         (check_effective_target_arm_neon_softfp_fp16_ok_nocache,
>         check_effective_target_arm_neon_softfp_fp16_ok,
>         add_options_for_arm_neon_softfp_fp16): New.
>
>

Hi,

I've noticed strange things with this new testcase.
I see it failing on target arm-none-linux-gnueabi  --with-mode arm
--with-cpu cortex-a9
and on target arm-none-eabi --with-mode arm --with-cpu cortex-a9


Looking at the logs, I see strange command lines when trying to
compile arm_neon_softfp_fp16_ok:
/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/xgcc
-B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/
-fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers
-fdiagnostics-color=never -mfloat-abi=softfp -mfpu=neon-fp16
-mfloat-abi=softfp -c -o arm_neon_softfp_fp16_ok21466.o
arm_neon_softfp_fp16_ok21466.c
arm_neon_softfp_fp16_ok21466.c:3:3: error: unknown type name
'float16x4_t'; did you mean 'float32x4_t'?
arm_neon_softfp_fp16_ok21466.c: In function 'foo':
arm_neon_softfp_fp16_ok21466.c:6:26: warning: implicit declaration of
function 'vcvt_f16_f32'; did you mean 'vcvt_u32_f32'?
[-Wimplicit-function-declaration]
compiler exited with status 1

[I don't know where the first 'mfloat-abi=softfp' comes from]
/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/xgcc
-B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/
-fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers
-fdiagnostics-color=never -mfloat-abi=softfp -mfloat-abi=softfp
-mfp16-format=ieee -c -o arm_neon_softfp_fp16_ok21466.o
arm_neon_softfp_fp16_ok21466.c
[succeeds]

then, when compiling the testcase:
/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/xgcc
-B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/
/gcc/testsuite/gcc.target/arm/pr88850-2.c -fno-diagnostics-show-caret
-fno-diagnostics-show-line-numbers -fdiagnostics-color=never -ansi
-pedantic-errors -O2 -march=armv7-a -fdump-rtl-final
-mfloat-abi=softfp -mfloat-abi=softfp -mfp16-format=ieee
-ffat-lto-objects -S -o pr88850-2.s
In file included from /gcc/testsuite/gcc.target/arm/pr88850-2.c:7:
/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/include/arm_neon.h:31:2:
error: #error "NEON intrinsics not available with the soft-float ABI.
Please use -mfloat-abi=softfp or -mfloat-abi=hard"
/gcc/testsuite/gcc.target/arm/pr88850-2.c:9:21: error: unknown type
name 'float16x4_t'
/gcc/testsuite/gcc.target/arm/pr88850-2.c:11:9: error: unknown type
name 'float16x4_t'

Why does the compiler think it's using float=abi=soft?


> The 02/13/2019 10:57, Kyrill Tkachov wrote:
> > Hi Tamar
> >
> > On 2/13/19 10:33 AM, Tamar Christina wrote:
> > > Hi All,
> > >
> > > The iterators ANY64 and ANY128 are used in various general split
> > > patterns and
> > > are supposed to contain any 64 bit and 128 bit modes respectively.
> > >
> > > For some reason these patterns had HI but not HF.  This adds HF so
> > > that general
> > > 64 and 128 bit splits are generated for these modes as well.  These
> > > are required
> > > by various split patterns that expect them to be there.
> > >
> > > Bootstrapped Regtested on aarch64-none-linux-gnu and <native regtest
> > > still running> issues.
> > >
> > Please do this on an arm-none-linux-gnueabihf target.
> >
> > Though I suspect this is just a placeholder from a boilerplate ;)
> >
> > > Ok for trunk?
> > >
> > > Thanks,
> > > Tamar
> > >
> > > gcc/ChangeLog:
> > >
> > > 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
> > >
> > >         PR target/88850
> > >         * config/arm/iterators.md (ANY64): Add V4HF,
> > >         (ANY128): Add V8HF.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
> > >
> > >         PR target/88850
> > >         * gcc.target/arm/pr88850-2.c: New test.
> > >
> > > --
> >
> > diff --git a/gcc/config/arm/iterators.md b/gcc/config/arm/iterators.md
> > index c33e572c3e89c3dc5848bd6b825d618481247558..4ac048a0c609273691c264c97ccf6cd47b43943b 100644
> > --- a/gcc/config/arm/iterators.md
> > +++ b/gcc/config/arm/iterators.md
> > @@ -24,11 +24,11 @@
> >   ;;----------------------------------------------------------------------------
> >
> >   ;; A list of modes that are exactly 64 bits in size. This is used to expand
> > -;; some splits that are the same for all modes when operating on ARM
> > +;; some splits that are the same for all modes when operating on ARM
> >   ;; registers.
> > -(define_mode_iterator ANY64 [DI DF V8QI V4HI V2SI V2SF])
> > +(define_mode_iterator ANY64 [DI DF V8QI V4HI V4HF V2SI V2SF])
> >
> > -(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V4SI V4SF])
> > +(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V8HF V4SI V4SF])
> >
> >
> > I see ANY128 is used in the move_hi_quad_<mode> and move_lo_quad_<mode> expanders in neon.md.
> > Does the use of HF modes need guarding by TARGET_NEON_FP16INST there?
> > Would be good to see a test exercising this change...
> >
> >
> >   ;; A list of integer modes that are up to one word long
> >   (define_mode_iterator QHSI [QI HI SI])
> > diff --git a/gcc/testsuite/gcc.target/arm/pr88850-2.c b/gcc/testsuite/gcc.target/arm/pr88850-2.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..fee2a0fe3a227ea4652bea2aa0e7335d20a7e9b5
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/arm/pr88850-2.c
> > @@ -0,0 +1,19 @@
> > +/* PR target/88850.  */
> > +/* { dg-do compile } */
> > +/* { dg-additional-options "-O2 -march=armv7-a -mfloat-abi=softfp -fdump-rtl-final" } */
> > +/* { dg-add-options arm_neon_fp16 } */
> > +/* { dg-require-effective-target arm_soft_ok } */
> > +/* { dg-require-effective-target arm_neon_fp16_ok } */
> > +
> >
> > arm_soft_ok checks if it's okay to add -mfloat-abi=soft, but you're adding softfp.
> > Is this right?
> >
> > Thanks,
> > Kyrill
> >
> > +#include <arm_neon.h>
> > +
> > +extern void c (int, float16x4_t);
> > +
> > +void a (float16x4_t b)
> > +{
> > +  c (0, b);
> > +}
> > +
> > +
> > +/* Check that these 64-bit moves are done in SI.  */
> > +/* { dg-final { scan-rtl-dump "_movsi_vfp" "final" } } *
> >
>
> --

Hi Christoph,

> 
> Looking at the logs, I see strange command lines when trying to compile
> arm_neon_softfp_fp16_ok:
> /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/xgcc
> -B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/
> -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers
> -fdiagnostics-color=never -mfloat-abi=softfp -mfpu=neon-fp16 -mfloat-
> abi=softfp -c -o arm_neon_softfp_fp16_ok21466.o
> arm_neon_softfp_fp16_ok21466.c
> arm_neon_softfp_fp16_ok21466.c:3:3: error: unknown type name
> 'float16x4_t'; did you mean 'float32x4_t'?
> arm_neon_softfp_fp16_ok21466.c: In function 'foo':
> arm_neon_softfp_fp16_ok21466.c:6:26: warning: implicit declaration of
> function 'vcvt_f16_f32'; did you mean 'vcvt_u32_f32'?
> [-Wimplicit-function-declaration]
> compiler exited with status 1
> 
> [I don't know where the first 'mfloat-abi=softfp' comes from] 

It comes from the check for check_effective_target_arm_neon_ok,
The tests first try to determine what options are required to get neon to work,
The arm_neon test has decided that -mfloat-abi=softfp was enough to get it to pass in your case.

> /aci-gcc-
> fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/xgcc
> -B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/
> -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers
> -fdiagnostics-color=never -mfloat-abi=softfp -mfloat-abi=softfp -mfp16-
> format=ieee -c -o arm_neon_softfp_fp16_ok21466.o
> arm_neon_softfp_fp16_ok21466.c [succeeds]
> 
> then, when compiling the testcase:
> /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/xgcc
> -B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/
> /gcc/testsuite/gcc.target/arm/pr88850-2.c -fno-diagnostics-show-caret -fno-
> diagnostics-show-line-numbers -fdiagnostics-color=never -ansi -pedantic-
> errors -O2 -march=armv7-a -fdump-rtl-final -mfloat-abi=softfp -mfloat-
> abi=softfp -mfp16-format=ieee -ffat-lto-objects -S -o pr88850-2.s In file
> included from /gcc/testsuite/gcc.target/arm/pr88850-2.c:7:
> /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-
> gnueabi/gcc3/gcc/include/arm_neon.h:31:2:
> error: #error "NEON intrinsics not available with the soft-float ABI.
> Please use -mfloat-abi=softfp or -mfloat-abi=hard"
> /gcc/testsuite/gcc.target/arm/pr88850-2.c:9:21: error: unknown type name
> 'float16x4_t'
> /gcc/testsuite/gcc.target/arm/pr88850-2.c:11:9: error: unknown type name
> 'float16x4_t'
> 
> Why does the compiler think it's using float=abi=soft?

The error is rather misleading, the likely reason is that the -mfpu isn't a neon one?
I guess the difference is the -march=armv7-a that's being set must be changing the fpu.

In my case, the feature test only passes on the last alternative which returns "-mfpu=neon-fp16 -mfloat-abi=softfp -mfp16-format=ieee " which is why it passes locally.

Forcing the -mfpu seems a bit wrong to me, so I guess the right solution is to remove the second alternative from the feature test and always explicitly test the fpu. I'll write up a patch.

Thanks,
Tamar

> 
> 
> > The 02/13/2019 10:57, Kyrill Tkachov wrote:
> > > Hi Tamar
> > >
> > > On 2/13/19 10:33 AM, Tamar Christina wrote:
> > > > Hi All,
> > > >
> > > > The iterators ANY64 and ANY128 are used in various general split
> > > > patterns and are supposed to contain any 64 bit and 128 bit modes
> > > > respectively.
> > > >
> > > > For some reason these patterns had HI but not HF.  This adds HF so
> > > > that general
> > > > 64 and 128 bit splits are generated for these modes as well.
> > > > These are required by various split patterns that expect them to
> > > > be there.
> > > >
> > > > Bootstrapped Regtested on aarch64-none-linux-gnu and <native
> > > > regtest still running> issues.
> > > >
> > > Please do this on an arm-none-linux-gnueabihf target.
> > >
> > > Though I suspect this is just a placeholder from a boilerplate ;)
> > >
> > > > Ok for trunk?
> > > >
> > > > Thanks,
> > > > Tamar
> > > >
> > > > gcc/ChangeLog:
> > > >
> > > > 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
> > > >
> > > >         PR target/88850
> > > >         * config/arm/iterators.md (ANY64): Add V4HF,
> > > >         (ANY128): Add V8HF.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > 2019-02-13  Tamar Christina  <tamar.christina@arm.com>
> > > >
> > > >         PR target/88850
> > > >         * gcc.target/arm/pr88850-2.c: New test.
> > > >
> > > > --
> > >
> > > diff --git a/gcc/config/arm/iterators.md
> > > b/gcc/config/arm/iterators.md index
> > >
> c33e572c3e89c3dc5848bd6b825d618481247558..4ac048a0c609273691c264c97c
> > > cf6cd47b43943b 100644
> > > --- a/gcc/config/arm/iterators.md
> > > +++ b/gcc/config/arm/iterators.md
> > > @@ -24,11 +24,11 @@
> > >
> > > ;;------------------------------------------------------------------
> > > ----------
> > >
> > >   ;; A list of modes that are exactly 64 bits in size. This is used
> > > to expand -;; some splits that are the same for all modes when
> > > operating on ARM
> > > +;; some splits that are the same for all modes when operating on
> > > +ARM
> > >   ;; registers.
> > > -(define_mode_iterator ANY64 [DI DF V8QI V4HI V2SI V2SF])
> > > +(define_mode_iterator ANY64 [DI DF V8QI V4HI V4HF V2SI V2SF])
> > >
> > > -(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V4SI V4SF])
> > > +(define_mode_iterator ANY128 [V2DI V2DF V16QI V8HI V8HF V4SI V4SF])
> > >
> > >
> > > I see ANY128 is used in the move_hi_quad_<mode> and
> move_lo_quad_<mode> expanders in neon.md.
> > > Does the use of HF modes need guarding by TARGET_NEON_FP16INST
> there?
> > > Would be good to see a test exercising this change...
> > >
> > >
> > >   ;; A list of integer modes that are up to one word long
> > >   (define_mode_iterator QHSI [QI HI SI]) diff --git
> > > a/gcc/testsuite/gcc.target/arm/pr88850-2.c
> > > b/gcc/testsuite/gcc.target/arm/pr88850-2.c
> > > new file mode 100644
> > > index
> > >
> 0000000000000000000000000000000000000000..fee2a0fe3a227ea4652bea2aa
> 0
> > > e7335d20a7e9b5
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/arm/pr88850-2.c
> > > @@ -0,0 +1,19 @@
> > > +/* PR target/88850.  */
> > > +/* { dg-do compile } */
> > > +/* { dg-additional-options "-O2 -march=armv7-a -mfloat-abi=softfp
> > > +-fdump-rtl-final" } */
> > > +/* { dg-add-options arm_neon_fp16 } */
> > > +/* { dg-require-effective-target arm_soft_ok } */
> > > +/* { dg-require-effective-target arm_neon_fp16_ok } */
> > > +
> > >
> > > arm_soft_ok checks if it's okay to add -mfloat-abi=soft, but you're adding
> softfp.
> > > Is this right?
> > >
> > > Thanks,
> > > Kyrill
> > >
> > > +#include <arm_neon.h>
> > > +
> > > +extern void c (int, float16x4_t);
> > > +
> > > +void a (float16x4_t b)
> > > +{
> > > +  c (0, b);
> > > +}
> > > +
> > > +
> > > +/* Check that these 64-bit moves are done in SI.  */
> > > +/* { dg-final { scan-rtl-dump "_movsi_vfp" "final" } } *
> > >
> >
> > --