On Mon, Jul 17, 2017 at 3:02 AM, Richard Biener <richard.guent...@gmail.com> wrote: > On Thu, Jul 13, 2017 at 6:18 AM, Andrew Pinski <pins...@gmail.com> wrote: >> On Wed, Jul 12, 2017 at 9:10 PM, Marc Glisse <marc.gli...@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
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; }