Re: [RFA][PATCH][PR rtl-optimization/47477] Type narrowing in match.pd

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Feb 11, 2015 at 6:05 PM, Jeff Law <law@redhat.com> wrote:
> On 02/11/15 03:56, Richard Biener wrote:
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>>> index 7f3816c..7a95029 100644
>>> --- a/gcc/ChangeLog
>>> +++ b/gcc/ChangeLog
>>> @@ -1,3 +1,8 @@
>>> +2015-02-10  Jeff Law  <law@redhat.com>
>>> +
>>> +       * match.pd (convert (plus/minus (convert @0) (convert @1): New
>>> +       simplifier to narrow arithmetic.
>>> +
>>
>>
>> Please reference PR47477 from here
>
> Doh.  Fixed.
>
>
>>
>>>   2015-02-10  Richard Biener  <rguenther@suse.de>
>>>
>>>          PR tree-optimization/64909
>>> diff --git a/gcc/match.pd b/gcc/match.pd
>>> index 81c4ee6..abc703e 100644
>>> --- a/gcc/match.pd
>>> +++ b/gcc/match.pd
>>> @@ -1018,3 +1018,21 @@ along with GCC; see the file COPYING3.  If not see
>>>      (logs (pows @0 @1))
>>>      (mult @1 (logs @0)))))
>>>
>>> +/* If we have a narrowing conversion of an arithmetic operation where
>>> +   both operands are widening conversions from the same type as the
>>> outer
>>> +   narrowing conversion.  Then convert the innermost operands to a
>>> suitable
>>> +   unsigned type (to avoid introducing undefined behaviour), perform the
>>> +   operation and convert the result to the desired type.
>>> +
>>> +   This narrows the arithmetic operation.  */
>>
>>
>> This is also a part of what shorten_binary_op does, so please refer to
>> that in the comment so we can eventually complete this from there
>> and remove shorten_binary_op.
>
> Done.  I've created a block comment meant to cover this pattern and any
> additions we need along the way to moving shortening/narrowing out of the
> front-ends.
>
>
>>
>>> +(for op (plus minus)
>>> +  (simplify
>>> +    (convert (op (convert@2 @0) (convert @1)))
>>> +    (if (TREE_TYPE (@0) == TREE_TYPE (@1)
>>> +         && TREE_TYPE (@0) == type
>>
>>
>> Otherwhere in match.pd we use
>>
>> (GIMPLE && types_compatible_p (TREE_TYPE (@0), TREE_TYPE (@1)))
>>   || (GENERIC && TREE_TYPE (@0) == TREE_TYPE (@1))
>>
>> for type equality checks.  And I think even for GENERIC you want
>> to ignore qualifiers and thus use TYPE_MAIN_VARIANT (TREE_TYPE (@0))
>> == TYPE_MAIN_VARAINT (TREE_TYPE (@1)).
>
> Seems reasonable.    Makes for some ugly formatting though.  It might make
> sense to factor that test so we don't end up repeating it senselessly.
>
>>
>>> +         && INTEGRAL_TYPE_P (type)
>>
>>
>> So instead of testing TREE_TYPE (@0) == type we probably want
>> to just assert that TYPE_PRECISON (TREE_TYPE (@0)) == TYPE_PRECISION
>> (type) to allow sign-changes.  Say for
>
> Yea, makes sense.
>
>>
>> short i, j;
>> (unsigned short) (i + j)
>>
>> we still want to narrow the widened i and j.
>
> Right.
>>
>>
>>> +         && TYPE_PRECISION (TREE_TYPE (@2)) > TYPE_PRECISION (TREE_TYPE
>>> (@0))
>>> +        /* This prevents infinite recursion.  */
>>> +        && unsigned_type_for (TREE_TYPE (@0)) != TREE_TYPE (@2))
>>
>>
>> How can it recurse with the type precision constraint right above?
>
> It may no longer be needed.  The precision check used to be >=, but that
> regressed some java codes.  So it got tightened in the last iteration before
> posting.
>
>>
>> Note that technically we don't need to perform the operation in an
>> unsigned
>> type iff TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0)).  Thus
>>
>>             (if (TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0))
>>               (convert (op @0 @1)))
>>             (with { tree utype = unsigned_type_for (TREE_TYPE (@0)); }
>>               (convert (op (convert:utype @0) (convert:utype @1))))))
>>
>> You can see that GCC exploits that with -fwrapv.  Maybe this
>> simplification should be split out into a separate pattern though,
>> tacking sign-changes for binary ops only.
>
> No strong opinion on separate pattern or integrating into existing pattern.

Yeah, I'd leave it as you did it for now.  Btw, -ENOPATCH.

Richard.

> jeff
>