diff mbox

[RFC,PR,target/39726,P4,regression] match.pd pattern to do type narrowing

Message ID CAFiYyc0_nvt+GAXmK8RX++XCjqvudaZtsK97XrtK3qiK+B8taA@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener Feb. 9, 2015, 1:42 p.m. UTC
On Mon, Feb 9, 2015 at 8:15 AM, Jeff Law <law@redhat.com> wrote:
> On 02/03/15 04:39, Richard Biener wrote:
>>>
>>> I found that explicit types were ignored in some cases.  It was
>>> frustrating to say the least.
>>
>>
>> Huh, that would be a bug.  Do you have a pattern where that happens?
>
> I'll have to recreate them.  In the mean time consider something else I'm
> playing with that causes an odd error from genmatch...
>
> /* If we have a narrowing conversion of an arithmetic or logical
>    operation where both are operands 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.  */
>   (simplify
>     (convert (plus (convert@2 @0) (convert @1)))
>     (if (TREE_TYPE (@0) == TREE_TYPE (@1)
>          && TREE_TYPE (@0) == type
>          && INTEGRAL_TYPE_P (type)
>          && TYPE_PRECISION (TREE_TYPE (@2)) >= TYPE_PRECISION (TREE_TYPE
> (@0)))
>       (with { tree utype = unsigned_type_for (TREE_TYPE (@0));}
>         (convert (plus (convert:utype @0) (convert:utype @1)))))))
>
> So given two narrow operands that get widened, added, and the final result
> narrowed back down to the original operand types.  Replace with convert the
> operands to an unsigned type (of same size as the operand), operate on them
> and convert to the final desired type.
>
> This happens to fix 47477 (P2 regression).  Works perfectly for the
> testcase.
>
>
> Of course we'd like to extend that to other operators...  So, adding the
> obvious for iterator...
>
> (for op (plus minus)
>   (simplify
>     (convert (op (convert@2 @0) (convert @1)))
>     (if (TREE_TYPE (@0) == TREE_TYPE (@1)
>          && TREE_TYPE (@0) == type
>          && INTEGRAL_TYPE_P (type)
>          && TYPE_PRECISION (TREE_TYPE (@2)) >= TYPE_PRECISION (TREE_TYPE
> (@0)))
>       (with { tree utype = unsigned_type_for (TREE_TYPE (@0));}
>         (convert (op (convert:utype @0) (convert:utype @1)))))))
>
>
> Which causes genmatch to barf:
>
> build/genmatch --gimple /home/gcc/GIT-2/gcc/gcc/match.pd \
>     > tmp-gimple-match.c
> genmatch: two conversions in a row
>
>
> Not only does genmatch barf, it doesn't give any indication what part of the
> .pd file it found objectionable.

Yeah, I'll have to assign locations to more places at some point.

But the following fixes your testcase, committed to trunk as obvious.

Richard.

2015-02-09  Richard Biener  <rguenther@suse.de>

        * genmatch.c (replace_id): Copy expr_type.



>
>
>

Comments

Jeff Law Feb. 9, 2015, 6:33 p.m. UTC | #1
On 02/09/15 06:42, Richard Biener wrote:
> On Mon, Feb 9, 2015 at 8:15 AM, Jeff Law <law@redhat.com> wrote:
>> On 02/03/15 04:39, Richard Biener wrote:
>>>>
>>>> I found that explicit types were ignored in some cases.  It was
>>>> frustrating to say the least.
>>>
>>>
>>> Huh, that would be a bug.  Do you have a pattern where that happens?
>>
>> I'll have to recreate them.  In the mean time consider something else I'm
>> playing with that causes an odd error from genmatch...
>>
>> /* If we have a narrowing conversion of an arithmetic or logical
>>     operation where both are operands 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.  */
>>    (simplify
>>      (convert (plus (convert@2 @0) (convert @1)))
>>      (if (TREE_TYPE (@0) == TREE_TYPE (@1)
>>           && TREE_TYPE (@0) == type
>>           && INTEGRAL_TYPE_P (type)
>>           && TYPE_PRECISION (TREE_TYPE (@2)) >= TYPE_PRECISION (TREE_TYPE
>> (@0)))
>>        (with { tree utype = unsigned_type_for (TREE_TYPE (@0));}
>>          (convert (plus (convert:utype @0) (convert:utype @1)))))))
>>
>> So given two narrow operands that get widened, added, and the final result
>> narrowed back down to the original operand types.  Replace with convert the
>> operands to an unsigned type (of same size as the operand), operate on them
>> and convert to the final desired type.
>>
>> This happens to fix 47477 (P2 regression).  Works perfectly for the
>> testcase.
>>
>>
>> Of course we'd like to extend that to other operators...  So, adding the
>> obvious for iterator...
>>
>> (for op (plus minus)
>>    (simplify
>>      (convert (op (convert@2 @0) (convert @1)))
>>      (if (TREE_TYPE (@0) == TREE_TYPE (@1)
>>           && TREE_TYPE (@0) == type
>>           && INTEGRAL_TYPE_P (type)
>>           && TYPE_PRECISION (TREE_TYPE (@2)) >= TYPE_PRECISION (TREE_TYPE
>> (@0)))
>>        (with { tree utype = unsigned_type_for (TREE_TYPE (@0));}
>>          (convert (op (convert:utype @0) (convert:utype @1)))))))
>>
>>
>> Which causes genmatch to barf:
>>
>> build/genmatch --gimple /home/gcc/GIT-2/gcc/gcc/match.pd \
>>      > tmp-gimple-match.c
>> genmatch: two conversions in a row
>>
>>
>> Not only does genmatch barf, it doesn't give any indication what part of the
>> .pd file it found objectionable.
>
> Yeah, I'll have to assign locations to more places at some point.
>
> But the following fixes your testcase, committed to trunk as obvious.
>
> Richard.
>
> 2015-02-09  Richard Biener  <rguenther@suse.de>
>
>          * genmatch.c (replace_id): Copy expr_type.
Cool, thanks!
jeff
diff mbox

Patch

Index: gcc/genmatch.c
===================================================================
--- gcc/genmatch.c      (revision 220540)
+++ gcc/genmatch.c      (working copy)
@@ -982,6 +982,7 @@  replace_id (operand *o, user_id *id, id_
     {
       expr *ne = new expr (e->operation == id ? with : e->operation,
                           e->is_commutative);
+      ne->expr_type = e->expr_type;
       for (unsigned i = 0; i < e->ops.length (); ++i)
        ne->append_op (replace_id (e->ops[i], id, with));
       return ne;