[RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq
Andrew Pinski
pinskia@gmail.com
Wed Jul 19 16:10:00 GMT 2017
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.
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
-------------- next part --------------
Index: fold-const.c
===================================================================
--- fold-const.c (revision 250333)
+++ fold-const.c (working copy)
@@ -3185,9 +3185,17 @@ operand_equal_p (const_tree arg0, const_
flags &= ~OEP_ADDRESS_OF;
return OP_SAME (0);
+ case BIT_INSERT_EXPR:
+ /* BIT_INSERT_EXPR has an implict operand as the type precision
+ of op1. Need to check to make sure they are the same. */
+ if (TREE_CODE (TREE_OPERAND (arg0, 0)) == INTEGER_CST
+ && TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (arg0, 1)))
+ != TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (arg1, 1))))
+ return false;
+ /* FALLTHRU */
+
case VEC_COND_EXPR:
case DOT_PROD_EXPR:
- case BIT_INSERT_EXPR:
return OP_SAME (0) && OP_SAME (1) && OP_SAME (2);
case MODIFY_EXPR:
Index: tree-ssa-sccvn.c
===================================================================
--- tree-ssa-sccvn.c (revision 250333)
+++ 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
+ && TREE_CODE (vno1->op[1]) == INTEGER_CST
+ && TYPE_PRECISION (TREE_TYPE (vno1->op[1]))
+ != TYPE_PRECISION (TREE_TYPE (vno2->op[1])))
+ return false;
+
return true;
}
More information about the Gcc-patches
mailing list