Patchwork [PATCH:,PR,target/44999] Replace "and r0, r0, #255" with uxtb in thumb2

login
register
mail settings
Submitter Carrot Wei
Date July 22, 2010, 7:11 a.m.
Message ID <AANLkTinA9B9PuMdEvoNG4h7wnRd9egzqBNuUChFMJ+S1@mail.gmail.com>
Download mbox | patch
Permalink /patch/59544/
State New
Headers show

Comments

Carrot Wei - July 22, 2010, 7:11 a.m.
In thumb2 "and r0, r0, #255" is 32 bit, uxtb is 16 bit and does the same
operation. This patch simply detect the situation in pattern "andsi3" and call
gen_thumb2_zero_extendqisi2_v6.

Tested on arm qemu.

ChangeLog:
2010-07-22  Wei Guozhi  <carrot@google.com>

        PR target/44999
        * config/arm/arm.md (andsi3): Change to zero extension if possible.
        * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name.

ChangeLog:
2010-07-22  Wei Guozhi  <carrot@google.com>

        PR target/44999
        * gcc.target/arm/pr44999.c: New testcase.
Carrot Wei - July 26, 2010, 6:39 a.m.
ping

On Thu, Jul 22, 2010 at 3:11 PM, Carrot Wei <carrot@google.com> wrote:
> In thumb2 "and r0, r0, #255" is 32 bit, uxtb is 16 bit and does the same
> operation. This patch simply detect the situation in pattern "andsi3" and call
> gen_thumb2_zero_extendqisi2_v6.
>
> Tested on arm qemu.
>
> ChangeLog:
> 2010-07-22  Wei Guozhi  <carrot@google.com>
>
>        PR target/44999
>        * config/arm/arm.md (andsi3): Change to zero extension if possible.
>        * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name.
>
> ChangeLog:
> 2010-07-22  Wei Guozhi  <carrot@google.com>
>
>        PR target/44999
>        * gcc.target/arm/pr44999.c: New testcase.
>
>
> Index: thumb2.md
> ===================================================================
> --- thumb2.md   (revision 162396)
> +++ thumb2.md   (working copy)
> @@ -970,7 +970,7 @@
>    (set_attr "neg_pool_range" "*,250")]
>  )
>
> -(define_insn "*thumb2_zero_extendqisi2_v6"
> +(define_insn "thumb2_zero_extendqisi2_v6"
>   [(set (match_operand:SI 0 "s_register_operand" "=r,r")
>        (zero_extend:SI (match_operand:QI 1 "nonimmediate_operand" "r,m")))]
>   "TARGET_THUMB2 && arm_arch6"
>
>
> Index: arm.md
> ===================================================================
> --- arm.md      (revision 162396)
> +++ arm.md      (working copy)
> @@ -1933,9 +1933,16 @@
>     {
>       if (GET_CODE (operands[2]) == CONST_INT)
>         {
> -          arm_split_constant (AND, SImode, NULL_RTX,
> -                             INTVAL (operands[2]), operands[0],
> -                             operands[1], optimize && can_create_pseudo_p ());
> +         if (INTVAL (operands[2]) == 255 && arm_arch6)
> +           {
> +             operands[1] = gen_rtx_SUBREG (QImode, operands[1], 0);
> +             gen_thumb2_zero_extendqisi2_v6 (operands[0], operands[1]);
> +           }
> +         else
> +           arm_split_constant (AND, SImode, NULL_RTX,
> +                               INTVAL (operands[2]), operands[0],
> +                               operands[1],
> +                               optimize && can_create_pseudo_p ());
>
>           DONE;
>         }
>
>
> Index: pr44999.c
> ===================================================================
> --- pr44999.c   (revision 0)
> +++ pr44999.c   (revision 0)
> @@ -0,0 +1,7 @@
> +/* { dg-options "-march=armv7-a -mthumb -Os" }  */
> +/* { dg-final { scan-assembler "uxtb" } } */
> +
> +int tp(int x, int y)
> +{
> +  return (x & 0xff) - (y & 0xffff);
> +}
>
Paolo Bonzini - July 26, 2010, 7:23 a.m.
On 07/26/2010 08:39 AM, Carrot Wei wrote:
> ping

After four days?  People are often on holiday in July.

The patch may be a reasonable approach, however I'd also check whether 
combine attempts this change, and if so why does it fail.

Paolo
Carrot Wei - July 26, 2010, 9:08 a.m.
I'm not an expert of combine, and I find the following comments in combine.c:

   We try to combine each pair of insns joined by a logical link.
   We also try to combine triples of insns A, B and C when
   C has a link back to B and B has a link back to A.

Only one insn is involved in the change, so combine should not check it.

thanks
Guozhi

On Mon, Jul 26, 2010 at 3:23 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
> On 07/26/2010 08:39 AM, Carrot Wei wrote:
>>
>> ping
>
> After four days?  People are often on holiday in July.
>
> The patch may be a reasonable approach, however I'd also check whether
> combine attempts this change, and if so why does it fail.
>
> Paolo
>
Paolo Bonzini - July 26, 2010, 9:22 a.m.
On 07/26/2010 11:08 AM, Carrot Wei wrote:
> I'm not an expert of combine, and I find the following comments in combine.c:
>
>     We try to combine each pair of insns joined by a logical link.
>     We also try to combine triples of insns A, B and C when
>     C has a link back to B and B has a link back to A.
>
> Only one insn is involved in the change, so combine should not check it.

Right, of course.  I check the i386 backend and it is also doing the 
same optimization as your patch does, i.e. without the help of the 
middle-end.

(It actually does it using a constraint instead of RTL, but that doesn't 
really matter and would be a bit harder to do for ARM).

Paolo
Carrot Wei - July 30, 2010, 6:31 a.m.
ARM maintainers, is this OK?

thanks
Guozhi

On Thu, Jul 22, 2010 at 3:11 PM, Carrot Wei <carrot@google.com> wrote:
> In thumb2 "and r0, r0, #255" is 32 bit, uxtb is 16 bit and does the same
> operation. This patch simply detect the situation in pattern "andsi3" and call
> gen_thumb2_zero_extendqisi2_v6.
>
> Tested on arm qemu.
>
> ChangeLog:
> 2010-07-22  Wei Guozhi  <carrot@google.com>
>
>        PR target/44999
>        * config/arm/arm.md (andsi3): Change to zero extension if possible.
>        * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name.
>
> ChangeLog:
> 2010-07-22  Wei Guozhi  <carrot@google.com>
>
>        PR target/44999
>        * gcc.target/arm/pr44999.c: New testcase.
>
>
> Index: thumb2.md
> ===================================================================
> --- thumb2.md   (revision 162396)
> +++ thumb2.md   (working copy)
> @@ -970,7 +970,7 @@
>    (set_attr "neg_pool_range" "*,250")]
>  )
>
> -(define_insn "*thumb2_zero_extendqisi2_v6"
> +(define_insn "thumb2_zero_extendqisi2_v6"
>   [(set (match_operand:SI 0 "s_register_operand" "=r,r")
>        (zero_extend:SI (match_operand:QI 1 "nonimmediate_operand" "r,m")))]
>   "TARGET_THUMB2 && arm_arch6"
>
>
> Index: arm.md
> ===================================================================
> --- arm.md      (revision 162396)
> +++ arm.md      (working copy)
> @@ -1933,9 +1933,16 @@
>     {
>       if (GET_CODE (operands[2]) == CONST_INT)
>         {
> -          arm_split_constant (AND, SImode, NULL_RTX,
> -                             INTVAL (operands[2]), operands[0],
> -                             operands[1], optimize && can_create_pseudo_p ());
> +         if (INTVAL (operands[2]) == 255 && arm_arch6)
> +           {
> +             operands[1] = gen_rtx_SUBREG (QImode, operands[1], 0);
> +             gen_thumb2_zero_extendqisi2_v6 (operands[0], operands[1]);
> +           }
> +         else
> +           arm_split_constant (AND, SImode, NULL_RTX,
> +                               INTVAL (operands[2]), operands[0],
> +                               operands[1],
> +                               optimize && can_create_pseudo_p ());
>
>           DONE;
>         }
>
>
> Index: pr44999.c
> ===================================================================
> --- pr44999.c   (revision 0)
> +++ pr44999.c   (revision 0)
> @@ -0,0 +1,7 @@
> +/* { dg-options "-march=armv7-a -mthumb -Os" }  */
> +/* { dg-final { scan-assembler "uxtb" } } */
> +
> +int tp(int x, int y)
> +{
> +  return (x & 0xff) - (y & 0xffff);
> +}
>
Carrot Wei - Aug. 9, 2010, 2:37 a.m.
ping

On Fri, Jul 30, 2010 at 2:31 PM, Carrot Wei <carrot@google.com> wrote:
> ARM maintainers, is this OK?
>
> thanks
> Guozhi
>
> On Thu, Jul 22, 2010 at 3:11 PM, Carrot Wei <carrot@google.com> wrote:
>> In thumb2 "and r0, r0, #255" is 32 bit, uxtb is 16 bit and does the same
>> operation. This patch simply detect the situation in pattern "andsi3" and call
>> gen_thumb2_zero_extendqisi2_v6.
>>
>> Tested on arm qemu.
>>
>> ChangeLog:
>> 2010-07-22  Wei Guozhi  <carrot@google.com>
>>
>>        PR target/44999
>>        * config/arm/arm.md (andsi3): Change to zero extension if possible.
>>        * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name.
>>
>> ChangeLog:
>> 2010-07-22  Wei Guozhi  <carrot@google.com>
>>
>>        PR target/44999
>>        * gcc.target/arm/pr44999.c: New testcase.
>>
>>
>> Index: thumb2.md
>> ===================================================================
>> --- thumb2.md   (revision 162396)
>> +++ thumb2.md   (working copy)
>> @@ -970,7 +970,7 @@
>>    (set_attr "neg_pool_range" "*,250")]
>>  )
>>
>> -(define_insn "*thumb2_zero_extendqisi2_v6"
>> +(define_insn "thumb2_zero_extendqisi2_v6"
>>   [(set (match_operand:SI 0 "s_register_operand" "=r,r")
>>        (zero_extend:SI (match_operand:QI 1 "nonimmediate_operand" "r,m")))]
>>   "TARGET_THUMB2 && arm_arch6"
>>
>>
>> Index: arm.md
>> ===================================================================
>> --- arm.md      (revision 162396)
>> +++ arm.md      (working copy)
>> @@ -1933,9 +1933,16 @@
>>     {
>>       if (GET_CODE (operands[2]) == CONST_INT)
>>         {
>> -          arm_split_constant (AND, SImode, NULL_RTX,
>> -                             INTVAL (operands[2]), operands[0],
>> -                             operands[1], optimize && can_create_pseudo_p ());
>> +         if (INTVAL (operands[2]) == 255 && arm_arch6)
>> +           {
>> +             operands[1] = gen_rtx_SUBREG (QImode, operands[1], 0);
>> +             gen_thumb2_zero_extendqisi2_v6 (operands[0], operands[1]);
>> +           }
>> +         else
>> +           arm_split_constant (AND, SImode, NULL_RTX,
>> +                               INTVAL (operands[2]), operands[0],
>> +                               operands[1],
>> +                               optimize && can_create_pseudo_p ());
>>
>>           DONE;
>>         }
>>
>>
>> Index: pr44999.c
>> ===================================================================
>> --- pr44999.c   (revision 0)
>> +++ pr44999.c   (revision 0)
>> @@ -0,0 +1,7 @@
>> +/* { dg-options "-march=armv7-a -mthumb -Os" }  */
>> +/* { dg-final { scan-assembler "uxtb" } } */
>> +
>> +int tp(int x, int y)
>> +{
>> +  return (x & 0xff) - (y & 0xffff);
>> +}
>>
>
Richard Earnshaw - Aug. 9, 2010, 10:30 a.m.
On Thu, 2010-07-22 at 15:11 +0800, Carrot Wei wrote:
> In thumb2 "and r0, r0, #255" is 32 bit, uxtb is 16 bit and does the same
> operation. This patch simply detect the situation in pattern "andsi3" and call
> gen_thumb2_zero_extendqisi2_v6.

> Index: arm.md
> ===================================================================
> --- arm.md      (revision 162396)
> +++ arm.md      (working copy)
> @@ -1933,9 +1933,16 @@
>      {
>        if (GET_CODE (operands[2]) == CONST_INT)
>          {
> -          arm_split_constant (AND, SImode, NULL_RTX,
> -                             INTVAL (operands[2]), operands[0],
> -                             operands[1], optimize && can_create_pseudo_p ());
> +         if (INTVAL (operands[2]) == 255 && arm_arch6)
> +           {
> +             operands[1] = gen_rtx_SUBREG (QImode, operands[1], 0);

No, never generate SUBREGs directly like this (you get the wrong byte
for a big-endian core).

Instead, you should use convert_to_mode().

>+/* { dg-options "-march=armv7-a -mthumb -Os" }  */
>+/* { dg-final { scan-assembler "uxtb" } } */

This is also wrong and will break if people are testing other CPU
permutations.  See thumb2-cmpneg2add-1.c for an example of how to do
this correctly.

R.
Nick Clifton - Aug. 10, 2010, 9:08 a.m.
Hi Carrot,

>> ARM maintainers, is this OK?

>>> ChangeLog:
>>> 2010-07-22  Wei Guozhi<carrot@google.com>
>>>
>>>         PR target/44999
>>>         * config/arm/arm.md (andsi3): Change to zero extension if possible.
>>>         * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name.
>>>
>>> ChangeLog:
>>> 2010-07-22  Wei Guozhi<carrot@google.com>
>>>
>>>         PR target/44999
>>>         * gcc.target/arm/pr44999.c: New testcase.

Approved - please apply.

Cheers
   Nick
Richard Earnshaw - Aug. 10, 2010, 2:08 p.m.
On Tue, 2010-08-10 at 10:08 +0100, Nick Clifton wrote:
> Hi Carrot,
> 
> >> ARM maintainers, is this OK?
> 
> >>> ChangeLog:
> >>> 2010-07-22  Wei Guozhi<carrot@google.com>
> >>>
> >>>         PR target/44999
> >>>         * config/arm/arm.md (andsi3): Change to zero extension if possible.
> >>>         * config/arm/thumb2.md (thumb2_zero_extendqisi2_v6): Change the name.
> >>>
> >>> ChangeLog:
> >>> 2010-07-22  Wei Guozhi<carrot@google.com>
> >>>
> >>>         PR target/44999
> >>>         * gcc.target/arm/pr44999.c: New testcase.
> 
> Approved - please apply.

Sorry, no it's not OK.  See my earlier review.

R.

Patch

Index: thumb2.md
===================================================================
--- thumb2.md   (revision 162396)
+++ thumb2.md   (working copy)
@@ -970,7 +970,7 @@ 
    (set_attr "neg_pool_range" "*,250")]
 )

-(define_insn "*thumb2_zero_extendqisi2_v6"
+(define_insn "thumb2_zero_extendqisi2_v6"
   [(set (match_operand:SI 0 "s_register_operand" "=r,r")
        (zero_extend:SI (match_operand:QI 1 "nonimmediate_operand" "r,m")))]
   "TARGET_THUMB2 && arm_arch6"


Index: arm.md
===================================================================
--- arm.md      (revision 162396)
+++ arm.md      (working copy)
@@ -1933,9 +1933,16 @@ 
     {
       if (GET_CODE (operands[2]) == CONST_INT)
         {
-          arm_split_constant (AND, SImode, NULL_RTX,
-                             INTVAL (operands[2]), operands[0],
-                             operands[1], optimize && can_create_pseudo_p ());
+         if (INTVAL (operands[2]) == 255 && arm_arch6)
+           {
+             operands[1] = gen_rtx_SUBREG (QImode, operands[1], 0);
+             gen_thumb2_zero_extendqisi2_v6 (operands[0], operands[1]);
+           }
+         else
+           arm_split_constant (AND, SImode, NULL_RTX,
+                               INTVAL (operands[2]), operands[0],
+                               operands[1],
+                               optimize && can_create_pseudo_p ());

           DONE;
         }

		
Index: pr44999.c
===================================================================
--- pr44999.c   (revision 0)
+++ pr44999.c   (revision 0)
@@ -0,0 +1,7 @@ 
+/* { dg-options "-march=armv7-a -mthumb -Os" }  */
+/* { dg-final { scan-assembler "uxtb" } } */
+
+int tp(int x, int y)
+{
+  return (x & 0xff) - (y & 0xffff);
+}