[PATCH][ARM] pr50193: ICE on a | (b << negative-constant)

Ramana Radhakrishnan ramana.radhakrishnan@linaro.org
Wed Sep 7 15:05:00 GMT 2011


On 5 September 2011 18:07, Andrew Stubbs <ams@codesourcery.com> wrote:
> On 01/09/11 17:21, Andrew Stubbs wrote:
>>
>> I wasn't sure how to find the mode of shift operand in the predicate
>> though, so I've assumed they're always the same size. How would one find
>> the proper mode in a predicate?
>
> OK, no reply, so I'm just going to assume we're dealing with 32-bit
> registers.
>
> Additionally, Richard Sandiford has pointed out that changing the predicate
> such that it is more restrictive than the constraints is a problem because
> reload apparently ignores the predicates under certain circumstances.
> Setting aside that that seems broken and wrong (what's the point in a
> predicate if you're just going to ignore it), this patch also creates a new
> constraint "Pm" that limits the range to match the predicate.



>
> Speaking of which, I've limited the constants to the range 1..31 (inclusive)
> because a) allowing zero seems like it would be counter-productive - it
> would be better to keep a zero shift as a separate operation that can be
> optimized away (probably not an issue, but there it is); and b) allowing
> zero would produce non-canonical assembler (which is not a problem now, but
> is still best avoided).
>
> I have a bootstrap test running now. Assuming that succeeds, is this ok?

This is a summary of what we discussed on IRC for others to also comment.

This doesn't capture the case of mult by power_of_two in arith_shiftsi .

So this testcase below

int * foo (int *x , int y)
{
   return x + (y << 24);
}

will fail .


There are 2 options as we discussed on IRC .
1. as you suggested a check in shift_operator for the shift amount.
2. add an alternative for the M constraint but enable it using
insn_enabled only if the mult_operator is valid.

This sort of cries to be rewritten with iterators and then we can
retain the mult case as a specific pattern only with the M constraint
but that is a significant rewrite.

> +;; in Thumb-2 state: Pj, PJ, Pm, Ps, Pt, Pu, Pv, Pw, Px, Py

Minor nit - Pm is valid for both ARM / Thumb2 state and not just
Thumb2 unlike the other constraints in the list above.

Hope this helps.

Ramana

>
> Andrew
>
>



More information about the Gcc-patches mailing list