Ping^3: use sync_add rather than sync_sub for CONST_INTs

Richard Sandiford richard@codesourcery.com
Sun Sep 9 11:23:00 GMT 2007


I'd like to ping David's patch (third time of asking):

    http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01417.html

It simply makes expand canonicalise sync_sub of a constant as sync_add,
just like expand does for plain "sub" and "add".  As it stands,
the target's sync_sub either has to accept a CONST_INT -- which for
patterns like alpha's and MIPS's would lead to noncanonical rtl --
or it has to unnecessarily force the constant into a register.

Richard

David Daney <ddaney@avtrex.com> writes:
> This patch is waiting for a middle-end maintainer review.
>
> Also note the follow-up comments in:
>
> http://gcc.gnu.org/ml/gcc-patches/2007-09/msg00090.html
>
> Thanks,
> David Daney
>
> David Daney wrote:
>> David Daney wrote:
>>> Richard Sandiford wrote:
>>>> David Daney <ddaney@avtrex.com> writes:
>>>>  
>>>>> While implementing the atomic memory operations for MIPS, I came 
>>>>> across a problem where immediate operands were forced into a 
>>>>> register when sync_sub* insns were expanded.  On MIPS there is no 
>>>>> minus instruction for immediate operands, instead this should be 
>>>>> expanded as plus with the negative value of the immediate operand.
>>>>>
>>>>> In expand_sync_operation() and expand_sync_fetch_operation() there 
>>>>> is code to handle conversion to plus if the minus insn does not 
>>>>> exits.  I enhanced this slightly so that it also does the conversion 
>>>>> if the predicate for sync_sub* does not match, but it does match for 
>>>>> the corresponding sync_add*.  This allows me to write sync_sub* 
>>>>> insns with a register predicate and have them used for non-immediate 
>>>>> operands, but an immediate operand results in conversion to sync_add*.
>>>>>
>>>>> Tested on x86_64-pc-linux-gnu all default languages both native and 
>>>>> -m32 with no regressions.  Also tested mips64-linux C only.
>>>>>
>>>>> :ADDPATCH middle-end:
>>>>>
>>>>> OK to commit?
>>>>>     
>>>>
>>>> FWIW, with (minus ... (const_int ...)) not being canonical rtl, I think
>>>> we should always use PLUS rather than MINUS if the operand is a 
>>>> CONST_INT.
>>>> E.g. the first thing expand_binop does is:
>>>>
>>>>   /* If subtracting an integer constant, convert this into an 
>>>> addition of
>>>>      the negated constant.  */
>>>>   if (binoptab == sub_optab && GET_CODE (op1) == CONST_INT)
>>>>     {
>>>>       op1 = negate_rtx (mode, op1);
>>>>       binoptab = add_optab;
>>>>     }
>>>>
>>>> and I think we should have a similar thing in expand_sync_operation
>>>> and expand_sync_fetch_operation.  As well as avoiding invalid
>>>> expressions, it has the advantage of folding the negation at
>>>> generation time, which I don't think the current version would.
>>>> (Sorry if that's wrong.)
>>>>   
>>>
>>> The attached new revision of the patch does it this way now.  Light 
>>> testing on MIPS shows that it has the same effect as the first 
>>> version.  Which from the point of view of my mips atomic memory 
>>> operations patch is sufficient.
>>>
>>> I was a little hesitant to do it this way the first time because it 
>>> has the potential of adversely impacting a (hypothetical) port where
>>>
>>> sync_sub<mode> is more efficient than sync_add<mode> the only place I 
>>> could think of where this would be plausible is for cases where 
>>> negating the value would cause it to no longer fit in the same number 
>>> of bits as the original value (-0x8000 on mips assuming there were a 
>>> subiu instruction).
>>>
>>> If deemed better than the previous version, I will fully test this.  
>>> It would however be nice to get feedback that this is really better 
>>> before exerting a lot more effort in testing.
>> 
>> Now tested on both i686-pc-linux-gnu and x86_64-pc-linux-gnu all default 
>> languages with no regressions found.
>> 
>> OK to commit?
>> 
>> 
>>>
>>> 2007-08-21  David Daney  <ddaney@avtrex.com>
>>>
>>>    * optabs.c (expand_sync_operation):  Use plus insn if minus
>>>    CONST_INT_P(val).
>>>    (expand_sync_fetch_operation):  Ditto.
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> Index: optabs.c
>>> ===================================================================
>>> --- optabs.c    (revision 127356)
>>> +++ optabs.c    (working copy)
>>> @@ -6444,7 +6444,7 @@ expand_sync_operation (rtx mem, rtx val,
>>>  
>>>      case MINUS:
>>>        icode = sync_sub_optab[mode];
>>> -      if (icode == CODE_FOR_nothing)
>>> +      if (icode == CODE_FOR_nothing || CONST_INT_P (val))
>>>      {
>>>        icode = sync_add_optab[mode];
>>>        if (icode != CODE_FOR_nothing)
>>> @@ -6544,7 +6544,8 @@ expand_sync_fetch_operation (rtx mem, rt
>>>      case MINUS:
>>>        old_code = sync_old_sub_optab[mode];
>>>        new_code = sync_new_sub_optab[mode];
>>> -      if (old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
>>> +      if ((old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
>>> +          || CONST_INT_P (val))
>>>      {
>>>        old_code = sync_old_add_optab[mode];
>>>        new_code = sync_new_add_optab[mode];
>> 



More information about the Gcc-patches mailing list