Patchwork [ARM] NEON DImode neg

login
register
mail settings
Submitter Andrew Stubbs
Date March 26, 2012, 10:14 a.m.
Message ID <4F704189.4010302@codesourcery.com>
Download mbox | patch
Permalink /patch/148692/
State New
Headers show

Comments

Andrew Stubbs - March 26, 2012, 10:14 a.m.
On 28/02/12 17:45, Andrew Stubbs wrote:
> Hi all,
>
> This patch adds a DImode negate pattern for NEON.
>
> Unfortunately, the NEON vneg instruction only supports vectors, not
> singletons, so there's no direct way to do it in DImode, and the
> compiler ends up moving the value back to core registers, negating it,
> and returning to NEON afterwards:
>
> fmrrd r2, r3, d16 @ int
> negs r2, r2
> sbc r3, r3, r3, lsl #1
> fmdrr d16, r2, r3 @ int
>
> The new patch does it entirely in NEON:
>
> vmov.i32 d17, #0 @ di
> vsub.i64 d16, d17, d16
>
> (Note that this is the result when combined with my recent patch for
> NEON DImode immediates. Without that you get a constant pool load.)

This updates fixes a bootstrap failure caused by an early clobber error. 
I've also got a native regression test running now.

OK?

Andrew
Andrew Stubbs - April 12, 2012, 2:57 p.m.
Ping.

On 26/03/12 11:14, Andrew Stubbs wrote:
> On 28/02/12 17:45, Andrew Stubbs wrote:
>> Hi all,
>>
>> This patch adds a DImode negate pattern for NEON.
>>
>> Unfortunately, the NEON vneg instruction only supports vectors, not
>> singletons, so there's no direct way to do it in DImode, and the
>> compiler ends up moving the value back to core registers, negating it,
>> and returning to NEON afterwards:
>>
>> fmrrd r2, r3, d16 @ int
>> negs r2, r2
>> sbc r3, r3, r3, lsl #1
>> fmdrr d16, r2, r3 @ int
>>
>> The new patch does it entirely in NEON:
>>
>> vmov.i32 d17, #0 @ di
>> vsub.i64 d16, d17, d16
>>
>> (Note that this is the result when combined with my recent patch for
>> NEON DImode immediates. Without that you get a constant pool load.)
>
> This updates fixes a bootstrap failure caused by an early clobber error.
> I've also got a native regression test running now.
>
> OK?
>
> Andrew
Richard Earnshaw - April 12, 2012, 3:48 p.m.
On 26/03/12 11:14, Andrew Stubbs wrote:
> On 28/02/12 17:45, Andrew Stubbs wrote:
>> Hi all,
>>
>> This patch adds a DImode negate pattern for NEON.
>>
>> Unfortunately, the NEON vneg instruction only supports vectors, not
>> singletons, so there's no direct way to do it in DImode, and the
>> compiler ends up moving the value back to core registers, negating it,
>> and returning to NEON afterwards:
>>
>> fmrrd r2, r3, d16 @ int
>> negs r2, r2
>> sbc r3, r3, r3, lsl #1
>> fmdrr d16, r2, r3 @ int
>>
>> The new patch does it entirely in NEON:
>>
>> vmov.i32 d17, #0 @ di
>> vsub.i64 d16, d17, d16
>>
>> (Note that this is the result when combined with my recent patch for
>> NEON DImode immediates. Without that you get a constant pool load.)
> 
> This updates fixes a bootstrap failure caused by an early clobber error. 
> I've also got a native regression test running now.
> 
> OK?
> 
> Andrew
> 
> 
> neon-neg64.patch
> 
> 
> 2012-03-26  Andrew Stubbs  <ams@codesourcery.com>
> 
> 	gcc/
> 	* config/arm/arm.md (negdi2): Use gen_negdi2_neon.
> 	* config/arm/neon.md (negdi2_neon): New insn.
> 	Also add splitters for core and NEON registers.
> 
> ---
>  gcc/config/arm/arm.md  |    8 +++++++-
>  gcc/config/arm/neon.md |   37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 1 deletions(-)
> 
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 751997f..f1dbbf7 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -4048,7 +4048,13 @@
>  	 (neg:DI (match_operand:DI 1 "s_register_operand" "")))
>      (clobber (reg:CC CC_REGNUM))])]
>    "TARGET_EITHER"
> -  ""
> +  {
> +    if (TARGET_NEON)
> +      {
> +        emit_insn (gen_negdi2_neon (operands[0], operands[1]));
> +	DONE;
> +      }
> +  }
>  )
>  
>  ;; The constraints here are to prevent a *partial* overlap (where %Q0 == %R1).
> diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
> index 3c88568..bf229a7 100644
> --- a/gcc/config/arm/neon.md
> +++ b/gcc/config/arm/neon.md
> @@ -922,6 +922,43 @@
>                      (const_string "neon_int_3")))]
>  )
>  
> +(define_insn "negdi2_neon"
> +  [(set (match_operand:DI 0 "s_register_operand"	 "= w,?r,?&r,?w")
> +	(neg:DI (match_operand:DI 1 "s_register_operand" "  w, 0,  r, w")))
> +   (clobber (match_scratch:DI 2				 "=&w, X,  X,&w"))
> +   (clobber (reg:CC CC_REGNUM))]
> +  "TARGET_NEON"
> +  "#"
> +  [(set_attr "length" "8")
> +   (set_attr "arch" "nota8,*,*,onlya8")]
> +)
> +

If negation in Neon needs a scratch register, it seems to me to be
somewhat odd that we're disparaging the ARM version.

Also, wouldn't it be sensible to support a variant that was
early-clobber on operand 0, but loaded immediate zero into that value first:

	vmov	Dd, #0
	vsub	Dd, Dd, Dm

That way you'll never need more than two registers, whereas today you
want three.


R.
Andrew Stubbs - April 12, 2012, 4:16 p.m.
On 12/04/12 16:48, Richard Earnshaw wrote:
> If negation in Neon needs a scratch register, it seems to me to be
> somewhat odd that we're disparaging the ARM version.

Good point, I'm really meaning to provide missing functionality in NEON 
to prevent unnecessary moves, rather than cause them, so I should remove 
the '?'s.

It probably doesn't need disparaging on A8, etiher?

> Also, wouldn't it be sensible to support a variant that was
> early-clobber on operand 0, but loaded immediate zero into that value first:
>
> 	vmov	Dd, #0
> 	vsub	Dd, Dd, Dm
>
> That way you'll never need more than two registers, whereas today you
> want three.

That does seem like a good option. I'll have a go and see what happens.

Andrew
Richard Earnshaw - April 12, 2012, 4:43 p.m.
On 12/04/12 17:16, Andrew Stubbs wrote:
> On 12/04/12 16:48, Richard Earnshaw wrote:
>> If negation in Neon needs a scratch register, it seems to me to be
>> somewhat odd that we're disparaging the ARM version.
> 
> Good point, I'm really meaning to provide missing functionality in NEON 
> to prevent unnecessary moves, rather than cause them, so I should remove 
> the '?'s.
> 
> It probably doesn't need disparaging on A8, etiher?

Possibly not.  Which then begs the question as to whether we need an A8
variant at all.

> 
>> Also, wouldn't it be sensible to support a variant that was
>> early-clobber on operand 0, but loaded immediate zero into that value first:
>>
>> 	vmov	Dd, #0
>> 	vsub	Dd, Dd, Dm
>>
>> That way you'll never need more than two registers, whereas today you
>> want three.
> 
> That does seem like a good option. I'll have a go and see what happens.
> 
> Andrew
>
Andrew Stubbs - April 14, 2012, 12:48 p.m.
On 12/04/12 16:48, Richard Earnshaw wrote:
> If negation in Neon needs a scratch register, it seems to me to be
> somewhat odd that we're disparaging the ARM version.
>
> Also, wouldn't it be sensible to support a variant that was
> early-clobber on operand 0, but loaded immediate zero into that value first:
>
> 	vmov	Dd, #0
> 	vsub	Dd, Dd, Dm
>
> That way you'll never need more than two registers, whereas today you
> want three.

This patch implements the changes you suggested.

I've done a full bootstrap and test and found no regressions.

OK?

Andrew

P.S. This patch can't actually be committed until my "NEON DImode 
immediate constants" patch is approved and committed. (Without that the 
load #0 needs a constant pool, and loading constants this late has a bug 
at -O0.)

Patch

2012-03-26  Andrew Stubbs  <ams@codesourcery.com>

	gcc/
	* config/arm/arm.md (negdi2): Use gen_negdi2_neon.
	* config/arm/neon.md (negdi2_neon): New insn.
	Also add splitters for core and NEON registers.

---
 gcc/config/arm/arm.md  |    8 +++++++-
 gcc/config/arm/neon.md |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 751997f..f1dbbf7 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -4048,7 +4048,13 @@ 
 	 (neg:DI (match_operand:DI 1 "s_register_operand" "")))
     (clobber (reg:CC CC_REGNUM))])]
   "TARGET_EITHER"
-  ""
+  {
+    if (TARGET_NEON)
+      {
+        emit_insn (gen_negdi2_neon (operands[0], operands[1]));
+	DONE;
+      }
+  }
 )
 
 ;; The constraints here are to prevent a *partial* overlap (where %Q0 == %R1).
diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
index 3c88568..bf229a7 100644
--- a/gcc/config/arm/neon.md
+++ b/gcc/config/arm/neon.md
@@ -922,6 +922,43 @@ 
                     (const_string "neon_int_3")))]
 )
 
+(define_insn "negdi2_neon"
+  [(set (match_operand:DI 0 "s_register_operand"	 "= w,?r,?&r,?w")
+	(neg:DI (match_operand:DI 1 "s_register_operand" "  w, 0,  r, w")))
+   (clobber (match_scratch:DI 2				 "=&w, X,  X,&w"))
+   (clobber (reg:CC CC_REGNUM))]
+  "TARGET_NEON"
+  "#"
+  [(set_attr "length" "8")
+   (set_attr "arch" "nota8,*,*,onlya8")]
+)
+
+; Split negdi2_neon for vfp registers
+(define_split
+  [(set (match_operand:DI 0 "s_register_operand" "")
+	(neg:DI (match_operand:DI 1 "s_register_operand" "")))
+   (clobber (match_scratch:DI 2 ""))
+   (clobber (reg:CC CC_REGNUM))]
+  "TARGET_NEON && reload_completed && IS_VFP_REGNUM (REGNO (operands[0]))"
+  [(set (match_dup 2) (const_int 0))
+   (parallel [(set (match_dup 0) (minus:DI (match_dup 2) (match_dup 1)))
+	      (clobber (reg:CC CC_REGNUM))])]
+  ""
+)
+
+; Split negdi2_neon for core registers
+(define_split
+  [(set (match_operand:DI 0 "s_register_operand" "")
+	(neg:DI (match_operand:DI 1 "s_register_operand" "")))
+   (clobber (match_scratch:DI 2 ""))
+   (clobber (reg:CC CC_REGNUM))]
+  "TARGET_32BIT && reload_completed
+   && arm_general_register_operand (operands[0], DImode)"
+  [(parallel [(set (match_dup 0) (neg:DI (match_dup 1)))
+	      (clobber (reg:CC CC_REGNUM))])]
+  ""
+)
+
 (define_insn "*umin<mode>3_neon"
   [(set (match_operand:VDQIW 0 "s_register_operand" "=w")
 	(umin:VDQIW (match_operand:VDQIW 1 "s_register_operand" "w")