[PATCH][ARM] Fix broken shift patterns

[Andrew Stubbs]

On 06/10/11 18:17, Paul Brook wrote:
>> I believe this patch to be nothing but an improvement over the current
>> state, and that a fix to the constraint problem should be a separate patch.
>>
>> In that basis, am I OK to commit?
>
> One minor nit:
>
>> (define_special_predicate "shift_operator"
>> ...
>> +	(ior (match_test "GET_CODE (XEXP (op, 1)) == CONST_INT
>> +			&&  ((unsigned HOST_WIDE_INT) INTVAL (XEXP (op, 1)))<  32")
>> +		(match_test "REG_P (XEXP (op, 1))"))))
>
> We're already enforcing the REG_P elsewhere, and it's only valid in some
> contexts, so I'd change this to:
>      (match_test "GET_CODE (XEXP (op, 1)) != CONST_INT
> 	|| ((unsigned HOST_WIDE_INT) INTVAL (XEXP (op, 1)))<  32")

Done, and attached.

> 3. Consistently accept both power-of-two and 0..31 for shifts.  Large shift
> counts give undefined results[1], so replace them with an arbitrary value
> (e.g. 0) during assembly output.  Argualy not an entirely "proper" fix, but I
> think it'll keep everything happy.

I think we need to be careful not to change the behaviour between 
different optimization levels and/or perturbations caused by minor code 
changes. I know this isn't a hard requirement for undefined behaviour, 
but I think it's still good practice.

In this case, I believe the hardware simply shifts the the value clean 
out of the register, and always returns a zero (or maybe -1 for 
ashiftrt?). I'm not sure what it does for rotate.

Anyway, my point is that I don't think that we could insert an immediate 
that had the same effect in all cases.

> For bonus points we should probably disallow MULT in the arm_shiftsi3 pattern,
> stop it interacting with the regulat mulsi3 pattern in undesirable ways.

How might that be a problem? Is it not the case that canonical forms 
already deals with this?

Anyway, it's easily achieved with an extra predicate.

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr50193.patch
Type: text/x-patch
Size: 3090 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20111007/cc70538c/attachment.bin>