[RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq

[RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

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.

[-- Attachment #2: fixbitinsert-vn.diff.txt --]
[-- Type: text/plain, Size: 675 bytes --]

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;
+
   return true;
 }
 

Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq

[Marc Glisse]

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?

-- 
Marc Glisse

Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq

[Andrew Pinski]

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);

Thanks,
Andrew Pinski

>
> --
> Marc Glisse

Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq

[Richard Biener]

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

Thanks,
Richard.

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

Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 4036 bytes --]

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

[-- Attachment #2: fixPRE.diff.txt --]
[-- Type: text/plain, Size: 1471 bytes --]

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;
 }
 

Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq

[Richard Biener]

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

Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 4798 bytes --]

On Wed, Jul 19, 2017 at 11:13 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> 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.

This is what I applied in the end. It is not op0 that needs to be
changed here but rather op1; I had a typo in the fold-const.c check.

Thanks,
Andrew

>
> 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
>

[-- Attachment #2: fixbitinsert-vn.diff.txt --]
[-- Type: text/plain, Size: 1532 bytes --]

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 250430)
+++ fold-const.c	(working copy)
@@ -3184,9 +3184,18 @@ 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, 1)) == INTEGER_CST
+	      && TREE_CODE (TREE_OPERAND (arg1, 1)) == 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 250430)
+++ 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;
 }