[PATCH][ARM] PR 65694: Properly sign-extend large numbers before passing to GEN_INT in arm_canonicalize_comparison

[Kyrill Tkachov]

Hi Jakub,

On 10/04/15 09:57, Jakub Jelinek wrote:
> On Fri, Apr 10, 2015 at 09:33:11AM +0100, Kyrill Tkachov wrote:
>> 2015-04-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
> Missing
> 	PR target/65694
> line here.

Fixed.

>
>>      * config/arm/arm.c (arm_canonicalize_comparison): Use ARM_SIGN_EXTEND
>>      when creating +1 values for SImode and trunc_int_for_mode for similar
>>      DImode operations.
>>
>> 2015-04-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
> Ditto.

Fixed.

>
>>      * g++.dg/torture/pr65694.C: New test.
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>> index 369cb67..5342b33 100644
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -4984,7 +4984,7 @@ arm_canonicalize_comparison (int *code, rtx *op0, rtx *op1,
>>   		  if (i != maxval
>>   		      && arm_const_double_by_immediates (GEN_INT (i + 1)))
>>   		    {
>> -		      *op1 = GEN_INT (i + 1);
>> +		      *op1 = GEN_INT (trunc_int_for_mode (i + 1, DImode));
>>   		      *code = *code == GT ? GE : LT;
>>   		      return;
>>   		    }
>> @@ -4994,7 +4994,7 @@ arm_canonicalize_comparison (int *code, rtx *op0, rtx *op1,
>>   		  if (i != ~((unsigned HOST_WIDE_INT) 0)
>>   		      && arm_const_double_by_immediates (GEN_INT (i + 1)))
>>   		    {
>> -		      *op1 = GEN_INT (i + 1);
>> +		      *op1 = GEN_INT (trunc_int_for_mode (i + 1, DImode));
> The above two aren't strictly necessary, HOST_WIDE_INT is always 64-bit, so
> is DImode, and GEN_INT takes HOST_WIDE_INT.

Yeah, those aren't strictly necessary, it's the SImode code that was
causing the ICE.

> You haven't changed it in the GEN_INT (i + 1) calls passed to
> arm_const_double_by_immediates anyway.
> I'd think you can leave those changes to cleanup in stage1 if desirable.
>
>> @@ -5047,7 +5047,7 @@ arm_canonicalize_comparison (int *code, rtx *op0, rtx *op1,
>>         if (i != maxval
>>   	  && (const_ok_for_arm (i + 1) || const_ok_for_arm (-(i + 1))))
>>   	{
>> -	  *op1 = GEN_INT (i + 1);
>> +	  *op1 = GEN_INT (ARM_SIGN_EXTEND (i + 1));
>>   	  *code = *code == GT ? GE : LT;
>>   	  return;
>>   	}
>> @@ -5069,7 +5069,7 @@ arm_canonicalize_comparison (int *code, rtx *op0, rtx *op1,
>>         if (i != ~((unsigned HOST_WIDE_INT) 0)
>>   	  && (const_ok_for_arm (i + 1) || const_ok_for_arm (-(i + 1))))
>>   	{
>> -	  *op1 = GEN_INT (i + 1);
>> +	  *op1 = GEN_INT (ARM_SIGN_EXTEND (i + 1));
>>   	  *code = *code == GTU ? GEU : LTU;
>>   	  return;
>>   	}
> This looks ok to me, but I'll defer the final word to ARM maintainers.
> That said, the ARM_SIGN_EXTEND macro could very well use some cleanup too
> now that HOST_WIDE_INT is always 64-bit and one can e.g. use
> HOST_WIDE_INT_{U,}C macros to build large constants.

But I'd think that's stage 1 material anyway.
Also, trunc_int_for_mode SImode would have worked here, I suspect.

Attached is updated patch without the DImode stuff and ChangeLog.

Thanks,
Kyrill

2015-04-09  Kyrylo Tkachov<kyrylo.tkachov@arm.com>

      PR target/65694
      * config/arm/arm.c (arm_canonicalize_comparison): Use ARM_SIGN_EXTEND
      when creating +1 values for SImode.

2015-04-09  Kyrylo Tkachov<kyrylo.tkachov@arm.com>

      PR target/65694
      * g++.dg/torture/pr65694.C: New test.



-------------- next part --------------
A non-text attachment was scrubbed...
Name: arm-comp-signextend.patch
Type: text/x-patch
Size: 5502 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20150410/2690f6d6/attachment.bin>