This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Andrew Pinski <pinskia at gmail dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 19 Jul 2017 20:13:20 +0200
- Subject: Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq
- Authentication-results: sourceware.org; auth=none
- References: <CA+=Sn1n4K9mW5ZH0ojZ3n6ZYh-denb=dUtojt3Mt=9AwdHnjHA@mail.gmail.com> <alpine.DEB.2.20.1707130557300.2554@stedding.saclay.inria.fr> <CA+=Sn1kkNUy+eR5RUu9ijJwoFivGN_V98EPE=zPkBVKzRkNUNw@mail.gmail.com> <CAFiYyc2YxoEx2YDVvfQ+uwc6uEvP_d15RofXtNVme=JSBNDQKw@mail.gmail.com> <CA+=Sn1mF5pMMHMmxvaFZ=wRw9pwxhU4w7_9B+9uxKPJn=G_-iA@mail.gmail.com>
On July 19, 2017 6:10:28 PM GMT+02:00, Andrew Pinski <pinskia@gmail.com> wrote:
>On Mon, Jul 17, 2017 at 3:02 AM, Richard Biener
><richard.guenther@gmail.com> wrote:
>> On Thu, Jul 13, 2017 at 6:18 AM, Andrew Pinski <pinskia@gmail.com>
>wrote:
>>> On Wed, Jul 12, 2017 at 9:10 PM, Marc Glisse <marc.glisse@inria.fr>
>wrote:
>>>> On Wed, 12 Jul 2017, Andrew Pinski wrote:
>>>>
>>>>> Hi,
>>>>> Unlike most other expressions, BIT_INSERT_EXPR has an implicit
>>>>> operand of the precision/size of the second operand. This means
>if we
>>>>> have an integer constant for the second operand and that compares
>to
>>>>> the same constant value, vn_nary_op_eq would return that these two
>>>>> expressions are the same. But in the case I was looking into the
>>>>> integer constants had different types, one with 1 bit precision
>and
>>>>> the other with 2 bit precision which means the BIT_INSERT_EXPR
>were
>>>>> not equal at all.
>>>>>
>>>>> This patches the problem by checking to see if BIT_INSERT_EXPR's
>>>>> operand 1's (second operand) type has different precision to
>return
>>>>> false.
>>>>>
>>>>> Is this the correct location or should we be checking for this
>>>>> differently? If this is the correct location, is the patch ok?
>>>>> Bootstrapped and tested on aarch64-linux-gnu with no regressions
>(and
>>>>> also tested with a few extra patches to expose BIT_INSERT_EXPR).
>>>>>
>>>>> Thanks,
>>>>> Andrew Pinski
>>>>>
>>>>> ChangeLog:
>>>>> * tree-ssa-sccvn.c (vn_nary_op_eq): Check BIT_INSERT_EXPR's
>operand 1
>>>>> to see if the types precision matches.
>>>>
>>>>
>>>> Hello,
>>>>
>>>> since BIT_INSERT_EXPR is implicitly an expression with 4 arguments,
>it makes
>>>> sense that we may need a few such special cases. But shouldn't the
>hash
>>>> function be in sync with the equality comparator? Does
>operand_equal_p need
>>>> the same?
>>>
>>> The hash function does not need to be exactly the same. The only
>>> requirement there is if vn_nary_op_eq returns true then the hash has
>>> to be the same. Now we could improve the hash by using the
>precision
>>> which will allow us not to compare as much in some cases.
>>>
>>> Yes operand_equal_p needs the same handling; I did not notice that
>>> until you mention it..
>>> Right now it does:
>>> case BIT_INSERT_EXPR:
>>> return OP_SAME (0) && OP_SAME (1) && OP_SAME (2);
>>
>> Aww. The issue is that operand_equal_p treats INTEGER_CSTs of
>different
>> type/precision but the same value as equal.
>>
>> Revisiting that, while a good idea, shouldn't block a fix here. So
>...
>>
>> Index: tree-ssa-sccvn.c
>> ===================================================================
>> --- tree-ssa-sccvn.c (revision 250159)
>> +++ tree-ssa-sccvn.c (working copy)
>> @@ -2636,6 +2636,14 @@ vn_nary_op_eq (const_vn_nary_op_t const
>> if (!expressions_equal_p (vno1->op[i], vno2->op[i]))
>> return false;
>>
>> + /* BIT_INSERT_EXPR has an implict operand as the type precision
>> + of op1. Need to check to make sure they are the same. */
>> + if (vno1->opcode == BIT_INSERT_EXPR)
>> + if (INTEGRAL_TYPE_P (TREE_TYPE (vno1->op[0]))
>> + && TYPE_PRECISION (TREE_TYPE (vno1->op[1]))
>> + != TYPE_PRECISION (TREE_TYPE (vno2->op[1])))
>> + return false;
>> +
>>
>> the case can be restricted to INTEGER_CST vno1->op[0] I think:
>>
>> if (vno1->opcode == BIT_INSERT_EXPR
>> && TREE_CODE (vno1->op[0]) == INTEGER_CST
>> && TYPE_PRECISION (....
>>
>> and yes, operand_equal_p needs a similar fix. Can you re-post with
>that added?
>
>Here is that with the changes you requested too.
>
>> Do you have a testcase?
>
>I don't have one which fails with the trunk. With lowering of
>bit-fields accesses (which I hope to submit soon; just getting in the
>required patches first), many testcases fail (bootstrap fails for the
>same reason too).
>
>OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions.
In the fold-const.c hunk you need to verify arg1 op0 is an INTEGER_CST. This is not necessary in sccvn because we passed operand_equal_p first.
OK with that change.
Richard.
>Thanks,
>Andrew
>
>ChangeLog:
>* tree-ssa-sccvn.c (vn_nary_op_eq): Check BIT_INSERT_EXPR's operand 1
>to see if the types precision matches.
>* fold-const.c (operand_equal_p): Likewise,
>
>
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> Andrew Pinski
>>>
>>>>
>>>> --
>>>> Marc Glisse