[ARM] Add support for ADDW and SUBW instructions

This is follow-up patch that fixes rtx costs for CONST_INT in PLUS 
pattern.  Original discussion is here: 
http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01427.html

On 06/06/11 13:15, Dmitry Plotnikov wrote:
> +	&&  (const_ok_for_op (INTVAL (x), outer)
> +	          || const_ok_for_op (~INTVAL (x), outer))))

The second call is redundant. const_ok_for_op should already do that.

Andrew

On 06/06/11 14:26, Dmitry Plotnikov wrote:
>         if (const_ok_for_arm (INTVAL (x))
> -	  || const_ok_for_arm (~INTVAL (x)))
> +	  || const_ok_for_arm (~INTVAL (x))
> +	  || (TARGET_THUMB2&&  outer == PLUS
> +	&&  (const_ok_for_op (INTVAL (x), outer))))

Sorry, I should have noticed before ...

This whole condition should be covered by a single call to 
const_ok_for_op. It already calls const_ok_for_arm internally.

Andrew

On 06/06/2011 05:33 PM, Andrew Stubbs wrote:
> On 06/06/11 14:26, Dmitry Plotnikov wrote:
>>         if (const_ok_for_arm (INTVAL (x))
>> -      || const_ok_for_arm (~INTVAL (x)))
>> +      || const_ok_for_arm (~INTVAL (x))
>> +      || (TARGET_THUMB2&&  outer == PLUS
>> + &&  (const_ok_for_op (INTVAL (x), outer))))
>
> Sorry, I should have noticed before ...
>
> This whole condition should be covered by a single call to 
> const_ok_for_op. It already calls const_ok_for_arm internally.
>
This condition is not covered by const_ok_for_op, because "outer" can be 
equal to other rtx codes, which are not covered in const_ok_for_op like 
IF_THEN_ELSE or MULT (I have several ICEs with these codes).  I don't 
know how to fix this correctly - should I add all codes to 
const_ok_for_op or maybe just replace default alternative from 
gcc_unreachable() to "return 0;" ?

On 06/06/11 15:26, Dmitry Plotnikov wrote:
> On 06/06/2011 05:33 PM, Andrew Stubbs wrote:
>> On 06/06/11 14:26, Dmitry Plotnikov wrote:
>>> if (const_ok_for_arm (INTVAL (x))
>>> - || const_ok_for_arm (~INTVAL (x)))
>>> + || const_ok_for_arm (~INTVAL (x))
>>> + || (TARGET_THUMB2&& outer == PLUS
>>> + && (const_ok_for_op (INTVAL (x), outer))))
>>
>> Sorry, I should have noticed before ...
>>
>> This whole condition should be covered by a single call to
>> const_ok_for_op. It already calls const_ok_for_arm internally.
>>
> This condition is not covered by const_ok_for_op, because "outer" can be
> equal to other rtx codes, which are not covered in const_ok_for_op like
> IF_THEN_ELSE or MULT (I have several ICEs with these codes). I don't
> know how to fix this correctly - should I add all codes to
> const_ok_for_op or maybe just replace default alternative from
> gcc_unreachable() to "return 0;" ?

That sounds reasonable to me - I mean, the constant is indeed not ok for 
those operations.

... but I'm not a maintainer ....

Andrew

On 06/06/11 15:31, Andrew Stubbs wrote:
> On 06/06/11 15:26, Dmitry Plotnikov wrote:
>> On 06/06/2011 05:33 PM, Andrew Stubbs wrote:
>>> On 06/06/11 14:26, Dmitry Plotnikov wrote:
>>>> if (const_ok_for_arm (INTVAL (x))
>>>> - || const_ok_for_arm (~INTVAL (x)))
>>>> + || const_ok_for_arm (~INTVAL (x))
>>>> + || (TARGET_THUMB2&& outer == PLUS
>>>> + && (const_ok_for_op (INTVAL (x), outer))))
>>>
>>> Sorry, I should have noticed before ...
>>>
>>> This whole condition should be covered by a single call to
>>> const_ok_for_op. It already calls const_ok_for_arm internally.
>>>
>> This condition is not covered by const_ok_for_op, because "outer" can be
>> equal to other rtx codes, which are not covered in const_ok_for_op like
>> IF_THEN_ELSE or MULT (I have several ICEs with these codes). I don't
>> know how to fix this correctly - should I add all codes to
>> const_ok_for_op or maybe just replace default alternative from
>> gcc_unreachable() to "return 0;" ?
> 
> That sounds reasonable to me - I mean, the constant is indeed not ok for 
> those operations.
> 
> ... but I'm not a maintainer ....
> 
> Andrew
> 
> 


const_ok_for_op has grown beyond it's original intended use.  A quick
look at it shows that a change along the way you describe would first
require fixing the assumption that if the constant matches
const_ok_for_arm in an unmodified form that it is OK universally.  That
might be a good idea anyway, I'm not entirely sure that that's correct
for all possible use cases these days.

It's also likely that it needs extending to handle some of these cases
you describe.  Having an outer of IF_THEN_ELSE most likely means this is
used in the case

(set reg if_then_else(cond) (op_or_const) (const))

for which the rules would be the same as if const were used directly in
a SET.

But it's also equally likely that we don't calculate the costs of
IF_THEN_ELSE very accurately anyway.

R.

R.