[RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq
diff mbox

Message ID CAFiYyc2YxoEx2YDVvfQ+uwc6uEvP_d15RofXtNVme=JSBNDQKw@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener July 17, 2017, 10:02 a.m. UTC
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 ...

  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?
Do you have a testcase?

Thanks,
Richard.

> Thanks,
> Andrew Pinski
>
>>
>> --
>> Marc Glisse

Patch
diff mbox

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: