* [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq @ 2017-07-13 1:19 Andrew Pinski 2017-07-13 4:14 ` Marc Glisse 0 siblings, 1 reply; 7+ messages in thread From: Andrew Pinski @ 2017-07-13 1:19 UTC (permalink / raw) To: GCC Patches [-- 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; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq 2017-07-13 1:19 [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq Andrew Pinski @ 2017-07-13 4:14 ` Marc Glisse 2017-07-13 4:18 ` Andrew Pinski 0 siblings, 1 reply; 7+ messages in thread From: Marc Glisse @ 2017-07-13 4:14 UTC (permalink / raw) To: Andrew Pinski; +Cc: GCC Patches 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq 2017-07-13 4:14 ` Marc Glisse @ 2017-07-13 4:18 ` Andrew Pinski 2017-07-17 10:03 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Andrew Pinski @ 2017-07-13 4:18 UTC (permalink / raw) To: GCC Patches 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq 2017-07-13 4:18 ` Andrew Pinski @ 2017-07-17 10:03 ` Richard Biener 2017-07-19 16:10 ` Andrew Pinski 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2017-07-17 10:03 UTC (permalink / raw) To: Andrew Pinski; +Cc: GCC Patches 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq 2017-07-17 10:03 ` Richard Biener @ 2017-07-19 16:10 ` Andrew Pinski 2017-07-19 18:13 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Andrew Pinski @ 2017-07-19 16:10 UTC (permalink / raw) To: Richard Biener; +Cc: GCC Patches [-- 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; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq 2017-07-19 16:10 ` Andrew Pinski @ 2017-07-19 18:13 ` Richard Biener 2017-07-21 17:16 ` Andrew Pinski 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2017-07-19 18:13 UTC (permalink / raw) To: Andrew Pinski; +Cc: GCC Patches 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq 2017-07-19 18:13 ` Richard Biener @ 2017-07-21 17:16 ` Andrew Pinski 0 siblings, 0 replies; 7+ messages in thread From: Andrew Pinski @ 2017-07-21 17:16 UTC (permalink / raw) To: Richard Biener; +Cc: GCC Patches [-- 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; } ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-07-21 17:16 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-13 1:19 [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq Andrew Pinski 2017-07-13 4:14 ` Marc Glisse 2017-07-13 4:18 ` Andrew Pinski 2017-07-17 10:03 ` Richard Biener 2017-07-19 16:10 ` Andrew Pinski 2017-07-19 18:13 ` Richard Biener 2017-07-21 17:16 ` Andrew Pinski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).