Re: [PATCH] Canonicalize compares in combine [3/3] ARM backend part

[Richard Earnshaw <rearnsha@arm.com>]

On 18 Jul 2011, at 07:15, "Chung-Lin Tang" <cltang@codesourcery.com> wrote:

> On 2011/6/15 09:12 PM, Richard Earnshaw wrote:
>> On 22/04/11 16:21, Chung-Lin Tang wrote:
>>> Hi Richard, this part's for you.
>>> 
>>> The ARM backend changes needed are very little after the prior patches,
>>> basically just a case in arm_canonicalize_comparison() to detect
>>> (zero_extend:SI (subreg:QI (reg:SI ...) 0)), and swap it into (and:SI
>>> (reg:SI) #255).
>>> 
>>> Had we not tried the combine modifications, this testcase probably could
>>> have also be solved by implementing another version of the corresponding
>>> *andsi3_compare0/_scratch patterns, with ZERO_EXTEND in the body, and
>>> "ands" in the output assembly. Maybe that's an acceptable solution too...
>>> 
>>> About the (ab)use of CANONICALIZE_COMPARISON, if it really should be
>>> another macro/hook, then this ARM patch will need updating, but the code
>>> should be similar.
>>> 
>>> Thanks,
>>> Chung-Lin
>>> 
>>> 
>>> 3-arm-parts.diff
>>> 
>>> 
>>> Index: config/arm/arm.c
>>> ===================================================================
>>> --- config/arm/arm.c    (revision 172860)
>>> +++ config/arm/arm.c    (working copy)
>>> @@ -3276,6 +3276,19 @@
>>>       return code;
>>>     }
>>> 
>>> +  /* If *op0 is (zero_extend:SI (subreg:QI (reg:SI) 0)) and comparing
>>> +     with const0_rtx, change it to (and:SI (reg:SI) (const_int 255)),
>>> +     to facilitate possible combining with a cmp into 'ands'.  */
>>> +  if (mode == SImode
>>> +      && GET_CODE (*op0) == ZERO_EXTEND
>>> +      && GET_CODE (XEXP (*op0, 0)) == SUBREG
>>> +      && GET_MODE (XEXP (*op0, 0)) == QImode
>>> +      && GET_MODE (SUBREG_REG (XEXP (*op0, 0))) == SImode
>>> +      && SUBREG_BYTE (XEXP (*op0, 0)) == 0
>>> +      && *op1 == const0_rtx)
>>> +    *op0 = gen_rtx_AND (SImode, SUBREG_REG (XEXP (*op0, 0)),
>>> +            GEN_INT (255));
>>> +
>> 
>> This is wrong for big-endian code.  You should use subreg_lowpart_p to
>> check the subreg expression (after you've checked that you do have a
>> subreg, of course).
>> 
>> R.
>> 
> 
> Hi Richard, thanks for catching that. I've updated the patch, and
> cross-tested again under both arm/armeb-Linux.
> 
> I changed the testcase to use -march=armv6t2 instead of armv6, as the
> latter makes the testcase FAIL when configured as --with-mode=thumb.
> 
> Is this now okay?
> 

The patch to arm.c is ok, but the change to the test is not as it will cause problems with multilib testing.  A better fix is to skip the test if the target is thumb1.

The other test needs a similar check as it seems to expect a movs instruction.

R.

> Thanks,
> Chung-Lin
> <uxtb-cmp.diff>