| Message ID | CA+=Sn1n4K9mW5ZH0ojZ3n6ZYh-denb=dUtojt3Mt=9AwdHnjHA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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?
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
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; }