Yes indeed ! here is a fixed patch. In strip_address_mutations we now have 3 if/else if statements with the same body which could be factorized in:
if ((GET_RTX_CLASS (code) == RTX_UNARY)
/* Things like SIGN_EXTEND, ZERO_EXTEND and TRUNCATE can be
used to convert between pointer sizes. */
|| (lsb_bitfield_op_p (*loc))
/* Bitfield operations [SIGN|ZERO]_EXTRACT from the least significant
bit can be used too. */
|| (code == AND && CONST_INT_P (XEXP (*loc, 1))))
/* (and ... (const_int -X)) is used to align to X bytes. */
loc = &XEXP (*loc, 0);
if you think that it doesn't affect too much the readability.
Many Thanks,
Yvan
On 11 September 2013 09:32, Richard Sandiford
<rdsandif...@googlemail.com> wrote:
> Yvan Roux <yvan.r...@linaro.org> writes:
>> @@ -5454,6 +5454,16 @@ strip_address_mutations (rtx *loc, enum rtx_code
>> *outer_code)
>> /* Things like SIGN_EXTEND, ZERO_EXTEND and TRUNCATE can be
>> used to convert between pointer sizes. */
>> loc = &XEXP (*loc, 0);
>> + else if (GET_RTX_CLASS (code) == RTX_BITFIELD_OPS)
>> + {
>> + /* Bitfield operations [SIGN|ZERO]_EXTRACT can be used too. */
>> + enum machine_mode mode = GET_MODE(*loc);
>> + unsigned HOST_WIDE_INT len = INTVAL (XEXP (*loc, 1));
>> + HOST_WIDE_INT pos = INTVAL (XEXP (*loc, 2));
>> +
>> + if (pos == (BITS_BIG_ENDIAN ? GET_MODE_PRECISION (mode) - len : 0))
>> + loc = &XEXP (*loc, 0);
>> + }
>
> This means that the other values of "pos" bypass the:
>
> else
> return loc;
>
> so you'll get an infinite loop. I think it would be neater to split
> this out into:
>
> /* Return true if X is a sign_extract or zero_extract from the least
> significant bit. */
>
> static bool
> lsb_bitfield_op_p (rtx X)
> {
> ...;
> }
>
> else if (lsb_bitfield_op_p (*loc))
> loc = &XEXP (*loc, 0);
>
> Looks good to me otherwise FWIW.
>
> Thanks,
> Richard
arm-lra.patch
Description: Binary data
