[AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Kugan]

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

This patch uses clobber with match_scratch instead of earlyclobber for
aarch64_lshr_sisd_or_int_<mode>3 so that RA can have more freedom in
selecting suitable register, as discussed in
http://thread.gmane.org/gmane.comp.gcc.patches/336162 and reported in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65139.

This is based on Maxim's patch. I have bootstrapped and regression
tested on aarch64-linux-gnu with no new regressions. Is this OK for trunk?

Thanks,
Kugan

gcc/ChangeLog:

2015-04-15  Kugan Vivekanandarajah  <kuganv@linaro.org>
	    Maxim Kuvyrkov  <maxim.kuvyrkov@linaro.org>

	PR target/65139
	* config/aarch64/aarch64.md (<optab><mode>3): Expand lshr with
	 gen_aarch64_lshr_sisd_or_int_<mode>3.
	(*aarch64_lshr_sisd_or_int_<mode>3): Rename to
	 aarch64_lshr_sisd_or_int_<mode>3 and use clobber with
	 match_scratch instead of early clobber.

[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 2317 bytes --]

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 534a862..07fa035 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -3277,6 +3277,12 @@
 	    DONE;
           }
       }
+
+    if (strcmp ("<optab>", "lshr") == 0)
+      {
+	emit_insn (gen_aarch64_lshr_sisd_or_int_<mode>3 (operands[0], operands[1], operands[2]));
+	DONE;
+      }
   }
 )
 
@@ -3361,11 +3367,12 @@
 )
 
 ;; Logical right shift using SISD or Integer instruction
-(define_insn "*aarch64_lshr_sisd_or_int_<mode>3"
-  [(set (match_operand:GPI 0 "register_operand" "=w,&w,r")
+(define_insn "aarch64_lshr_sisd_or_int_<mode>3"
+  [(set (match_operand:GPI 0 "register_operand" "=w,w,r")
         (lshiftrt:GPI
           (match_operand:GPI 1 "register_operand" "w,w,r")
-          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>" "Us<cmode>,w,rUs<cmode>")))]
+          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>" "Us<cmode>,w,rUs<cmode>")))
+   (clobber (match_scratch:QI 3 "=X,w,X"))]
   ""
   "@
    ushr\t%<rtn>0<vas>, %<rtn>1<vas>, %2
@@ -3379,30 +3386,28 @@
   [(set (match_operand:DI 0 "aarch64_simd_register")
         (lshiftrt:DI
            (match_operand:DI 1 "aarch64_simd_register")
-           (match_operand:QI 2 "aarch64_simd_register")))]
+           (match_operand:QI 2 "aarch64_simd_register")))
+   (clobber (match_scratch:QI 3))]
   "TARGET_SIMD && reload_completed"
   [(set (match_dup 3)
         (unspec:QI [(match_dup 2)] UNSPEC_SISD_NEG))
    (set (match_dup 0)
         (unspec:DI [(match_dup 1) (match_dup 3)] UNSPEC_SISD_USHL))]
-  {
-    operands[3] = gen_lowpart (QImode, operands[0]);
-  }
+  ""
 )
 
 (define_split
   [(set (match_operand:SI 0 "aarch64_simd_register")
         (lshiftrt:SI
            (match_operand:SI 1 "aarch64_simd_register")
-           (match_operand:QI 2 "aarch64_simd_register")))]
+           (match_operand:QI 2 "aarch64_simd_register")))
+   (clobber (match_scratch:QI 3))]
   "TARGET_SIMD && reload_completed"
   [(set (match_dup 3)
         (unspec:QI [(match_dup 2)] UNSPEC_SISD_NEG))
    (set (match_dup 0)
         (unspec:SI [(match_dup 1) (match_dup 3)] UNSPEC_USHL_2S))]
-  {
-    operands[3] = gen_lowpart (QImode, operands[0]);
-  }
+  ""
 )
 
 ;; Arithmetic right shift using SISD or Integer instruction

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Maxim Kuvyrkov]

> On Apr 15, 2015, at 12:41 AM, Kugan <kugan.vivekanandarajah@linaro.org> wrote:
> 
> This patch uses clobber with match_scratch instead of earlyclobber for
> aarch64_lshr_sisd_or_int_<mode>3 so that RA can have more freedom in
> selecting suitable register, as discussed in
> http://thread.gmane.org/gmane.comp.gcc.patches/336162 and reported in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65139.
> 
> This is based on Maxim's patch. I have bootstrapped and regression
> tested on aarch64-linux-gnu with no new regressions. Is this OK for trunk?
> 
> Thanks,
> Kugan
> 
> gcc/ChangeLog:
> 
> 2015-04-15  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 	    Maxim Kuvyrkov  <maxim.kuvyrkov@linaro.org>
> 
> 	PR target/65139
> 	* config/aarch64/aarch64.md (<optab><mode>3): Expand lshr with
> 	 gen_aarch64_lshr_sisd_or_int_<mode>3.
> 	(*aarch64_lshr_sisd_or_int_<mode>3): Rename to
> 	 aarch64_lshr_sisd_or_int_<mode>3 and use clobber with
> 	 match_scratch instead of early clobber.
> <p.txt>

For the benefit of other reviewers -- I have reviewed this patch internally and it looks OK.

Kugan, did you audit other patterns in aarch64.md to see if any other early-clobbers can be optimized in this way?

Thank you,

--
Maxim Kuvyrkov
www.linaro.org

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Richard Earnshaw]

On 14/04/15 22:41, Kugan wrote:
> This patch uses clobber with match_scratch instead of earlyclobber for
> aarch64_lshr_sisd_or_int_<mode>3 so that RA can have more freedom in
> selecting suitable register, as discussed in
> http://thread.gmane.org/gmane.comp.gcc.patches/336162 and reported in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65139.
>
> This is based on Maxim's patch. I have bootstrapped and regression
> tested on aarch64-linux-gnu with no new regressions. Is this OK for trunk?
>
> Thanks,
> Kugan
>
> gcc/ChangeLog:
>
> 2015-04-15  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 	    Maxim Kuvyrkov  <maxim.kuvyrkov@linaro.org>
>
> 	PR target/65139
> 	* config/aarch64/aarch64.md (<optab><mode>3): Expand lshr with
> 	 gen_aarch64_lshr_sisd_or_int_<mode>3.
> 	(*aarch64_lshr_sisd_or_int_<mode>3): Rename to
> 	 aarch64_lshr_sisd_or_int_<mode>3 and use clobber with
> 	 match_scratch instead of early clobber.
>

+    if (strcmp ("<optab>", "lshr") == 0)
+      {


This can't be the best way to match the operation type.  Yes, I know 
that the comparison is a compile time invariant, but there must be an 
attribute of optab (or one can be created for it) that would make the 
test trivial and not rely on the compiler optimizing the strcmp away.

R.

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Jakub Jelinek]

On Wed, Apr 15, 2015 at 01:18:36PM +0100, Richard Earnshaw wrote:
> On 14/04/15 22:41, Kugan wrote:
> >This patch uses clobber with match_scratch instead of earlyclobber for
> >aarch64_lshr_sisd_or_int_<mode>3 so that RA can have more freedom in
> >selecting suitable register, as discussed in
> >http://thread.gmane.org/gmane.comp.gcc.patches/336162 and reported in
> >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65139.
> >
> >This is based on Maxim's patch. I have bootstrapped and regression
> >tested on aarch64-linux-gnu with no new regressions. Is this OK for trunk?
> >
> >Thanks,
> >Kugan
> >
> >gcc/ChangeLog:
> >
> >2015-04-15  Kugan Vivekanandarajah  <kuganv@linaro.org>
> >	    Maxim Kuvyrkov  <maxim.kuvyrkov@linaro.org>
> >
> >	PR target/65139
> >	* config/aarch64/aarch64.md (<optab><mode>3): Expand lshr with
> >	 gen_aarch64_lshr_sisd_or_int_<mode>3.
> >	(*aarch64_lshr_sisd_or_int_<mode>3): Rename to
> >	 aarch64_lshr_sisd_or_int_<mode>3 and use clobber with
> >	 match_scratch instead of early clobber.
> >
> 
> +    if (strcmp ("<optab>", "lshr") == 0)
> +      {
> 
> 
> This can't be the best way to match the operation type.  Yes, I know that
> the comparison is a compile time invariant, but there must be an attribute
> of optab (or one can be created for it) that would make the test trivial and
> not rely on the compiler optimizing the strcmp away.

Perhaps just
  if (<CODE> == LSHIFTRT)
    {
?

	Jakub

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Kugan]

On 15/04/15 21:59, Maxim Kuvyrkov wrote:
>> On Apr 15, 2015, at 12:41 AM, Kugan <kugan.vivekanandarajah@linaro.org> wrote:
>>
>> This patch uses clobber with match_scratch instead of earlyclobber for
>> aarch64_lshr_sisd_or_int_<mode>3 so that RA can have more freedom in
>> selecting suitable register, as discussed in
>> http://thread.gmane.org/gmane.comp.gcc.patches/336162 and reported in
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65139.
>>
>> This is based on Maxim's patch. I have bootstrapped and regression
>> tested on aarch64-linux-gnu with no new regressions. Is this OK for trunk?
>>
>> Thanks,
>> Kugan
>>
>> gcc/ChangeLog:
>>
>> 2015-04-15  Kugan Vivekanandarajah  <kuganv@linaro.org>
>> 	    Maxim Kuvyrkov  <maxim.kuvyrkov@linaro.org>
>>
>> 	PR target/65139
>> 	* config/aarch64/aarch64.md (<optab><mode>3): Expand lshr with
>> 	 gen_aarch64_lshr_sisd_or_int_<mode>3.
>> 	(*aarch64_lshr_sisd_or_int_<mode>3): Rename to
>> 	 aarch64_lshr_sisd_or_int_<mode>3 and use clobber with
>> 	 match_scratch instead of early clobber.
>> <p.txt>
> 
> For the benefit of other reviewers -- I have reviewed this patch internally and it looks OK.
> 
> Kugan, did you audit other patterns in aarch64.md to see if any other early-clobbers can be optimized in this way?

Not yet. I will after this patch is gone through the review.

Thanks,
Kugan

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Kugan]

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



On 15/04/15 22:32, Jakub Jelinek wrote:
> On Wed, Apr 15, 2015 at 01:18:36PM +0100, Richard Earnshaw wrote:
>> On 14/04/15 22:41, Kugan wrote:
>>> This patch uses clobber with match_scratch instead of earlyclobber for
>>> aarch64_lshr_sisd_or_int_<mode>3 so that RA can have more freedom in
>>> selecting suitable register, as discussed in
>>> http://thread.gmane.org/gmane.comp.gcc.patches/336162 and reported in
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65139.
>>>
>>> This is based on Maxim's patch. I have bootstrapped and regression
>>> tested on aarch64-linux-gnu with no new regressions. Is this OK for trunk?
>>>
>>> Thanks,
>>> Kugan
>>>
>>> gcc/ChangeLog:
>>>
>>> 2015-04-15  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>> 	    Maxim Kuvyrkov  <maxim.kuvyrkov@linaro.org>
>>>
>>> 	PR target/65139
>>> 	* config/aarch64/aarch64.md (<optab><mode>3): Expand lshr with
>>> 	 gen_aarch64_lshr_sisd_or_int_<mode>3.
>>> 	(*aarch64_lshr_sisd_or_int_<mode>3): Rename to
>>> 	 aarch64_lshr_sisd_or_int_<mode>3 and use clobber with
>>> 	 match_scratch instead of early clobber.
>>>
>>
>> +    if (strcmp ("<optab>", "lshr") == 0)
>> +      {
>>
>>
>> This can't be the best way to match the operation type.  Yes, I know that
>> the comparison is a compile time invariant, but there must be an attribute
>> of optab (or one can be created for it) that would make the test trivial and
>> not rely on the compiler optimizing the strcmp away.
> 
> Perhaps just
>   if (<CODE> == LSHIFTRT)
>     {

Thanks. I have now changed it as above and reran the regression.

Richard, is this OK to you?

Kugan

[-- Attachment #2: shift.txt --]
[-- Type: text/plain, Size: 2318 bytes --]

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 534a862..4a16529 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -3277,6 +3277,12 @@
 	    DONE;
           }
       }
+
+    if (<CODE> == LSHIFTRT)
+      {
+        emit_insn (gen_aarch64_lshr_sisd_or_int_<mode>3 (operands[0], operands[1], operands[2]));
+        DONE;
+      }
   }
 )
 
@@ -3361,11 +3367,12 @@
 )
 
 ;; Logical right shift using SISD or Integer instruction
-(define_insn "*aarch64_lshr_sisd_or_int_<mode>3"
-  [(set (match_operand:GPI 0 "register_operand" "=w,&w,r")
+(define_insn "aarch64_lshr_sisd_or_int_<mode>3"
+  [(set (match_operand:GPI 0 "register_operand" "=w,w,r")
         (lshiftrt:GPI
           (match_operand:GPI 1 "register_operand" "w,w,r")
-          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>" "Us<cmode>,w,rUs<cmode>")))]
+          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>" "Us<cmode>,w,rUs<cmode>")))
+   (clobber (match_scratch:QI 3 "=X,w,X"))]
   ""
   "@
    ushr\t%<rtn>0<vas>, %<rtn>1<vas>, %2
@@ -3379,30 +3386,28 @@
   [(set (match_operand:DI 0 "aarch64_simd_register")
         (lshiftrt:DI
            (match_operand:DI 1 "aarch64_simd_register")
-           (match_operand:QI 2 "aarch64_simd_register")))]
+           (match_operand:QI 2 "aarch64_simd_register")))
+   (clobber (match_scratch:QI 3))]
   "TARGET_SIMD && reload_completed"
   [(set (match_dup 3)
         (unspec:QI [(match_dup 2)] UNSPEC_SISD_NEG))
    (set (match_dup 0)
         (unspec:DI [(match_dup 1) (match_dup 3)] UNSPEC_SISD_USHL))]
-  {
-    operands[3] = gen_lowpart (QImode, operands[0]);
-  }
+  ""
 )
 
 (define_split
   [(set (match_operand:SI 0 "aarch64_simd_register")
         (lshiftrt:SI
            (match_operand:SI 1 "aarch64_simd_register")
-           (match_operand:QI 2 "aarch64_simd_register")))]
+           (match_operand:QI 2 "aarch64_simd_register")))
+   (clobber (match_scratch:QI 3))]
   "TARGET_SIMD && reload_completed"
   [(set (match_dup 3)
         (unspec:QI [(match_dup 2)] UNSPEC_SISD_NEG))
    (set (match_dup 0)
         (unspec:SI [(match_dup 1) (match_dup 3)] UNSPEC_USHL_2S))]
-  {
-    operands[3] = gen_lowpart (QImode, operands[0]);
-  }
+  ""
 )
 
 ;; Arithmetic right shift using SISD or Integer instruction

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Jakub Jelinek]

On Thu, Apr 16, 2015 at 08:27:24AM +1000, Kugan wrote:
> +    if (<CODE> == LSHIFTRT)
> +      {
> +        emit_insn (gen_aarch64_lshr_sisd_or_int_<mode>3 (operands[0], operands[1], operands[2]));

That is way too long line, please wrap it.

> +        DONE;
> +      }
>    }
>  )
>  
> @@ -3361,11 +3367,12 @@
>  )
>  
>  ;; Logical right shift using SISD or Integer instruction
> -(define_insn "*aarch64_lshr_sisd_or_int_<mode>3"
> -  [(set (match_operand:GPI 0 "register_operand" "=w,&w,r")
> +(define_insn "aarch64_lshr_sisd_or_int_<mode>3"
> +  [(set (match_operand:GPI 0 "register_operand" "=w,w,r")
>          (lshiftrt:GPI
>            (match_operand:GPI 1 "register_operand" "w,w,r")
> -          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>" "Us<cmode>,w,rUs<cmode>")))]
> +          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>" "Us<cmode>,w,rUs<cmode>")))

Though, this one too...

	Jakub

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Kugan]

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



On 16/04/15 08:32, Jakub Jelinek wrote:
> On Thu, Apr 16, 2015 at 08:27:24AM +1000, Kugan wrote:
>> +    if (<CODE> == LSHIFTRT)
>> +      {
>> +        emit_insn (gen_aarch64_lshr_sisd_or_int_<mode>3 (operands[0], operands[1], operands[2]));
> 
> That is way too long line, please wrap it.
> 
>> +        DONE;
>> +      }
>>    }
>>  )
>>  
>> @@ -3361,11 +3367,12 @@
>>  )
>>  
>>  ;; Logical right shift using SISD or Integer instruction
>> -(define_insn "*aarch64_lshr_sisd_or_int_<mode>3"
>> -  [(set (match_operand:GPI 0 "register_operand" "=w,&w,r")
>> +(define_insn "aarch64_lshr_sisd_or_int_<mode>3"
>> +  [(set (match_operand:GPI 0 "register_operand" "=w,w,r")
>>          (lshiftrt:GPI
>>            (match_operand:GPI 1 "register_operand" "w,w,r")
>> -          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>" "Us<cmode>,w,rUs<cmode>")))]
>> +          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>" "Us<cmode>,w,rUs<cmode>")))
> 
> Though, this one too...
> 

Fixed in the attached patch.

Thanks,
Kugan

[-- Attachment #2: shift2.txt --]
[-- Type: text/plain, Size: 2465 bytes --]

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 534a862..72a9f05 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -3277,6 +3277,14 @@
 	    DONE;
           }
       }
+
+    if (<CODE> == LSHIFTRT)
+      {
+        emit_insn (gen_aarch64_lshr_sisd_or_int_<mode>3 (operands[0],
+                                                         operands[1],
+                                                         operands[2]));
+        DONE;
+      }
   }
 )
 
@@ -3361,11 +3369,13 @@
 )
 
 ;; Logical right shift using SISD or Integer instruction
-(define_insn "*aarch64_lshr_sisd_or_int_<mode>3"
-  [(set (match_operand:GPI 0 "register_operand" "=w,&w,r")
+(define_insn "aarch64_lshr_sisd_or_int_<mode>3"
+  [(set (match_operand:GPI 0 "register_operand" "=w,w,r")
         (lshiftrt:GPI
           (match_operand:GPI 1 "register_operand" "w,w,r")
-          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>" "Us<cmode>,w,rUs<cmode>")))]
+          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>"
+                              "Us<cmode>,w,rUs<cmode>")))
+   (clobber (match_scratch:QI 3 "=X,w,X"))]
   ""
   "@
    ushr\t%<rtn>0<vas>, %<rtn>1<vas>, %2
@@ -3379,30 +3389,28 @@
   [(set (match_operand:DI 0 "aarch64_simd_register")
         (lshiftrt:DI
            (match_operand:DI 1 "aarch64_simd_register")
-           (match_operand:QI 2 "aarch64_simd_register")))]
+           (match_operand:QI 2 "aarch64_simd_register")))
+   (clobber (match_scratch:QI 3))]
   "TARGET_SIMD && reload_completed"
   [(set (match_dup 3)
         (unspec:QI [(match_dup 2)] UNSPEC_SISD_NEG))
    (set (match_dup 0)
         (unspec:DI [(match_dup 1) (match_dup 3)] UNSPEC_SISD_USHL))]
-  {
-    operands[3] = gen_lowpart (QImode, operands[0]);
-  }
+  ""
 )
 
 (define_split
   [(set (match_operand:SI 0 "aarch64_simd_register")
         (lshiftrt:SI
            (match_operand:SI 1 "aarch64_simd_register")
-           (match_operand:QI 2 "aarch64_simd_register")))]
+           (match_operand:QI 2 "aarch64_simd_register")))
+   (clobber (match_scratch:QI 3))]
   "TARGET_SIMD && reload_completed"
   [(set (match_dup 3)
         (unspec:QI [(match_dup 2)] UNSPEC_SISD_NEG))
    (set (match_dup 0)
         (unspec:SI [(match_dup 1) (match_dup 3)] UNSPEC_USHL_2S))]
-  {
-    operands[3] = gen_lowpart (QImode, operands[0]);
-  }
+  ""
 )
 
 ;; Arithmetic right shift using SISD or Integer instruction

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Richard Earnshaw]

On 16/04/15 00:00, Kugan wrote:
> 
> 
> On 16/04/15 08:32, Jakub Jelinek wrote:
>> On Thu, Apr 16, 2015 at 08:27:24AM +1000, Kugan wrote:
>>> +    if (<CODE> == LSHIFTRT)
>>> +      {
>>> +        emit_insn (gen_aarch64_lshr_sisd_or_int_<mode>3 (operands[0], operands[1], operands[2]));
>>
>> That is way too long line, please wrap it.
>>
>>> +        DONE;
>>> +      }
>>>    }
>>>  )
>>>  
>>> @@ -3361,11 +3367,12 @@
>>>  )
>>>  
>>>  ;; Logical right shift using SISD or Integer instruction
>>> -(define_insn "*aarch64_lshr_sisd_or_int_<mode>3"
>>> -  [(set (match_operand:GPI 0 "register_operand" "=w,&w,r")
>>> +(define_insn "aarch64_lshr_sisd_or_int_<mode>3"
>>> +  [(set (match_operand:GPI 0 "register_operand" "=w,w,r")
>>>          (lshiftrt:GPI
>>>            (match_operand:GPI 1 "register_operand" "w,w,r")
>>> -          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>" "Us<cmode>,w,rUs<cmode>")))]
>>> +          (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>" "Us<cmode>,w,rUs<cmode>")))
>>
>> Though, this one too...
>>
> 
> Fixed in the attached patch.
> 
> Thanks,
> Kugan
> 


Not ok.

You need to ensure that your scratch register cannot overlap op1, since
the scratch is written before op1 is read.

R.

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Jakub Jelinek]

On Sat, Apr 18, 2015 at 03:07:16PM +0100, Richard Earnshaw wrote:
> You need to ensure that your scratch register cannot overlap op1, since
> the scratch is written before op1 is read.

-   (clobber (match_scratch:QI 3 "=X,w,X"))]
+   (clobber (match_scratch:QI 3 "=X,&w,X"))]

incremental diff should ensure that, right?

	Jakub

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Richard Earnshaw]

On 18/04/15 16:13, Jakub Jelinek wrote:
> On Sat, Apr 18, 2015 at 03:07:16PM +0100, Richard Earnshaw wrote:
>> You need to ensure that your scratch register cannot overlap op1, since
>> the scratch is written before op1 is read.
> 
> -   (clobber (match_scratch:QI 3 "=X,w,X"))]
> +   (clobber (match_scratch:QI 3 "=X,&w,X"))]
> 
> incremental diff should ensure that, right?
> 
> 	Jakub
> 


Sorry, where in the patch is that hunk?

I see just:

+   (clobber (match_scratch:QI 3 "=X,w,X"))]

And why would early clobbering the scratch be notably better than the
original?

R.

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Maxim Kuvyrkov]

> On Apr 18, 2015, at 8:21 PM, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
> 
> On 18/04/15 16:13, Jakub Jelinek wrote:
>> On Sat, Apr 18, 2015 at 03:07:16PM +0100, Richard Earnshaw wrote:
>>> You need to ensure that your scratch register cannot overlap op1, since
>>> the scratch is written before op1 is read.
>> 
>> -   (clobber (match_scratch:QI 3 "=X,w,X"))]
>> +   (clobber (match_scratch:QI 3 "=X,&w,X"))]
>> 
>> incremental diff should ensure that, right?
>> 
>> 	Jakub
>> 
> 
> 
> Sorry, where in the patch is that hunk?
> 
> I see just:
> 
> +   (clobber (match_scratch:QI 3 "=X,w,X"))]

Jakub's suggestion is an incremental patch on top of Kugan's.

> 
> And why would early clobbering the scratch be notably better than the
> original?
> 

It will still be better.  With this patch we want to allow RA freedom to optimally handle both of the following cases:

1. operand[1] dies after the instruction.  In this case we want operand[0] and operand[1] to be assigned to the same reg, and operand[3] to be assigned to a different register to provide a temporary.  In this case we don't care whether operand[3] is early-clobber or not.  This case is not optimally handled with current insn patterns.

2. operand[1] lives on after the instruction.  In this case we want operand[0] and operand[3] to be assigned to the same reg, and not clobber operand[1].  By marking operand[3] early-clobber we ensure that operand[1] is in a different register from what operand[0] and operand[3] were assigned to.  This case should be handled equally well before and after the patch.

My understanding is that Kugan's patch with Jakub's fix on top satisfy both of these cases.
 
--
Maxim Kuvyrkov
www.linaro.org

Re: [AArch64][PR65139] use clobber with match_scratch for aarch64_lshr_sisd_or_int_<mode>3

[Richard Earnshaw]

On 18/04/15 19:17, Maxim Kuvyrkov wrote:
>> On Apr 18, 2015, at 8:21 PM, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>>
>> On 18/04/15 16:13, Jakub Jelinek wrote:
>>> On Sat, Apr 18, 2015 at 03:07:16PM +0100, Richard Earnshaw wrote:
>>>> You need to ensure that your scratch register cannot overlap op1, since
>>>> the scratch is written before op1 is read.
>>>
>>> -   (clobber (match_scratch:QI 3 "=X,w,X"))]
>>> +   (clobber (match_scratch:QI 3 "=X,&w,X"))]
>>>
>>> incremental diff should ensure that, right?
>>>
>>> 	Jakub
>>>
>>
>>
>> Sorry, where in the patch is that hunk?
>>
>> I see just:
>>
>> +   (clobber (match_scratch:QI 3 "=X,w,X"))]
> 
> Jakub's suggestion is an incremental patch on top of Kugan's.
> 

Ah, sorry, I though he was implying it was already in the patch somewhere.

>>
>> And why would early clobbering the scratch be notably better than the
>> original?
>>
> 
> It will still be better.  With this patch we want to allow RA freedom to optimally handle both of the following cases:
> 
> 1. operand[1] dies after the instruction.  In this case we want operand[0] and operand[1] to be assigned to the same reg, and operand[3] to be assigned to a different register to provide a temporary.  In this case we don't care whether operand[3] is early-clobber or not.  This case is not optimally handled with current insn patterns.
> 
> 2. operand[1] lives on after the instruction.  In this case we want operand[0] and operand[3] to be assigned to the same reg, and not clobber operand[1].  By marking operand[3] early-clobber we ensure that operand[1] is in a different register from what operand[0] and operand[3] were assigned to.  This case should be handled equally well before and after the patch.
> 
> My understanding is that Kugan's patch with Jakub's fix on top satisfy both of these cases.
>  

I still don't think it handles all cases efficiently.  If we really want
the result in a different register from both of the inputs, then now we
need two registers for the results, one for the result and another for
the temporary.  In that case we could have used the result register as
the scratch, but now we can't.

Maybe we can provide two alternatives, one that early-clobbers the
result register but doesn't need a scratch and one that doesn't
early-clobber the result, but does need a scratch.

So something like

(define_insn "aarch64_lshr_sisd_or_int_<mode>3"
  [(set (match_operand:GPI 0 "register_operand" "=w,&w,w,r")
         (lshiftrt:GPI
           (match_operand:GPI 1 "register_operand" "w,w,w,r")
           (match_operand:QI 2 "aarch64_reg_or_shift_imm_<mode>"
                              "Us<cmode>,w,w,rUs<cmode>")))
   (clobber (match_scratch:QI 3 "=X,X,w,X"))]

... but I haven't tested any of that.

I would also note the conversation in
https://gcc.gnu.org/ml/gcc/2015-04/msg00240.html.  That seems to suggest
we should be wary of using scratch sequences since the register
allocator doesn't account for them properly.

R.

> --
> Maxim Kuvyrkov
> www.linaro.org
>