On Wed, Jul 19, 2017 at 11:13 AM, Richard Biener wrote: > On July 19, 2017 6:10:28 PM GMT+02:00, Andrew Pinski wrote: >>On Mon, Jul 17, 2017 at 3:02 AM, Richard Biener >> wrote: >>> On Thu, Jul 13, 2017 at 6:18 AM, Andrew Pinski >>wrote: >>>> On Wed, Jul 12, 2017 at 9:10 PM, Marc Glisse >>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 >