| Message ID | CA+=Sn1mF5pMMHMmxvaFZ=wRw9pwxhU4w7_9B+9uxKPJn=G_-iA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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; }