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

Hi Richard

The following patch has been modified as your suggestion. And it
passed the testing on qemu.





On Mon, Aug 9, 2010 at 6:30 PM, Richard Earnshaw <rearnsha@arm.com> wrote:
>
> 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.
>
>

On Wed, 2010-08-11 at 09:13 +0800, Carrot Wei wrote:
> Hi Richard
> 
> The following patch has been modified as your suggestion. And it
> passed the testing on qemu.
> 
> 
> Index: pr44999.c
> ===================================================================
> --- pr44999.c   (revision 0)
> +++ pr44999.c   (revision 0)
> @@ -0,0 +1,9 @@
> +/* Use UXTB to extract the lowest byte.  */
> +/* { dg-options "-mthumb -Os" } */
> +/* { dg-require-effective-target arm_thumb2_ok } */
> +/* { dg-final { scan-assembler "uxtb" } } */
> +
> +int tp(int x, int y)
> +{
> +  return (x & 0xff) - (y & 0xffff);
> +}
> 
> Index: thumb2.md
> ===================================================================
> --- thumb2.md   (revision 163046)
> +++ thumb2.md   (working copy)
> @@ -907,7 +907,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 163046)
> +++ arm.md      (working copy)
> @@ -1947,9 +1947,17 @@
>      {
>        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] = convert_to_mode (QImode, operands[1], 1);
> +             emit_insn (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;
>          }
> 


This is OK.

R.

> > +             operands[1] = convert_to_mode (QImode, operands[1], 1);
> > +             emit_insn (gen_thumb2_zero_extendqisi2_v6 (operands[0],
> > +                                                        operands[1]));
> > +           }
> >          
> >          }
> 
> This is OK.

No it's not. This code is inside if (TARGET_32BIT). Using thumb specific 
patterns in ARM mode is just plain wrong.

Even ignoring that, I don't think this is the right fix. Why do we need this 
for Thumb mode, and not ARM mode? Why extendqi and not extendhi?
Previous toolchains got this right, and still get this right in ARM mode. I 
suspect you're actually papering over a different bug. 

Paul