| Message ID | ecf4c116-9697-71c7-c04f-d563b8c1f64d@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | include MEM_REF type in tree dumps (PR 90676) | expand |
A patch with a ChangeLog this time is attached. On 6/1/19 9:53 AM, Martin Sebor wrote: > I spent a bunch of time the other day trying to understand why > the second of the two assignments below to a char array was > apparently not being done by trunk > > a[0] = 1; > a[1] = 0; > > The optimized GIMPLE dump simply shows: > > MEM[(char *)&a] = 1; > > when in the past it showed: > > MEM[(char[2] *)&a2] = 1; > > After some debugging I figured out that this is the result of > the store merging pass transforming the two assignments into > one: > > *(short int *)a = 1; > > and the MEM_REF dump mentioning only the type of the second > operand and not the type of the access. > > To avoid this confusion the attached patch adds to the dump > a cast to the MEM_REF type for accesses whose size is not equal > to the size of the operand (when the sizes are the same no new > cast is prepended). The effect is that with store merging in > effect, the dump for the above becomes > > MEM[(short int *)(char *)&a] = 1; > > This should make both the size and the type of the access clear > and help avoid the confusion. The output isn't the same as in > earlier releases because because the access really is done via > a short pointer and not as an array of char. > > There is more detail in MEM_REF that could be included here but > it seems that the size of the access is essential to interpreting > the dumps. > > Tested on x86_64-linux with only minimal testsuite fallout. > > Martin
On Sat, Jun 1, 2019 at 5:53 PM Martin Sebor <msebor@gmail.com> wrote: > > I spent a bunch of time the other day trying to understand why > the second of the two assignments below to a char array was > apparently not being done by trunk > > a[0] = 1; > a[1] = 0; > > The optimized GIMPLE dump simply shows: > > MEM[(char *)&a] = 1; > > when in the past it showed: > > MEM[(char[2] *)&a2] = 1; > > After some debugging I figured out that this is the result of > the store merging pass transforming the two assignments into > one: > > *(short int *)a = 1; > > and the MEM_REF dump mentioning only the type of the second > operand and not the type of the access. > > To avoid this confusion the attached patch adds to the dump > a cast to the MEM_REF type for accesses whose size is not equal > to the size of the operand (when the sizes are the same no new > cast is prepended). The effect is that with store merging in > effect, the dump for the above becomes > > MEM[(short int *)(char *)&a] = 1; I think this is confusing syntax. Iff you absolutely refuse to make the -gimple dump the default for MEM_REF and you insist on fixing this issue then please follow how we dump VIEW_CONVERT_EXPR which is the only other tree code we dump the access type, thus MEM<short int *>[(char *)&a] = 1; btw, -gimple (iff only applied to MEM_REF) would have dumped __MEM <short int, 8> ((char *)&a2) = 1; also including the effective alignment of the access. Your variant suggests that the alignment is that of short int which is wrong. Yes, there's testsuite fallout I guess mostly about () vs. [] and spacing (the GIMPLE FE doesn't insist on spacing before/after <>). So please do not change MEM_REF dumping in this way. Richard. > This should make both the size and the type of the access clear > and help avoid the confusion. The output isn't the same as in > earlier releases because because the access really is done via > a short pointer and not as an array of char. > > There is more detail in MEM_REF that could be included here but > it seems that the size of the access is essential to interpreting > the dumps. > > Tested on x86_64-linux with only minimal testsuite fallout. > > Martin
On Mon, Jun 03, 2019 at 10:36:42AM +0200, Richard Biener wrote: > > To avoid this confusion the attached patch adds to the dump > > a cast to the MEM_REF type for accesses whose size is not equal > > to the size of the operand (when the sizes are the same no new > > cast is prepended). The effect is that with store merging in > > effect, the dump for the above becomes > > > > MEM[(short int *)(char *)&a] = 1; > > I think this is confusing syntax. Iff you absolutely refuse to > make the -gimple dump the default for MEM_REF and you insist > on fixing this issue then please follow how we dump VIEW_CONVERT_EXPR > which is the only other tree code we dump the access type, thus I must say I prefer the current MEM[ over the -gimple for human readable dumps. > MEM<short int *>[(char *)&a] = 1; Wouldn't that be MEM<short int>[(char *)&a] instead? Couldn't we do it only if the TREE_TYPE (TREE_TYPE (TREE_OPERAND (mem, 1))) is not compatible with TREE_TYPE (mem), so keep what we were doing in most cases? Jakub
On Mon, Jun 3, 2019 at 10:57 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Jun 03, 2019 at 10:36:42AM +0200, Richard Biener wrote: > > > To avoid this confusion the attached patch adds to the dump > > > a cast to the MEM_REF type for accesses whose size is not equal > > > to the size of the operand (when the sizes are the same no new > > > cast is prepended). The effect is that with store merging in > > > effect, the dump for the above becomes > > > > > > MEM[(short int *)(char *)&a] = 1; > > > > I think this is confusing syntax. Iff you absolutely refuse to > > make the -gimple dump the default for MEM_REF and you insist > > on fixing this issue then please follow how we dump VIEW_CONVERT_EXPR > > which is the only other tree code we dump the access type, thus > > I must say I prefer the current MEM[ over the -gimple for human readable > dumps. Sure, but then why ask for all information to be present when in the cases you are curious you can look at -gimple dumps? A similar thing I've hacked the pretty printer locally for debugging in the past is alignment info. > > MEM<short int *>[(char *)&a] = 1; > > Wouldn't that be > MEM<short int>[(char *)&a] instead? Err, yes. > Couldn't we do it only if the TREE_TYPE (TREE_TYPE (TREE_OPERAND (mem, 1))) > is not compatible with TREE_TYPE (mem), so keep what we were doing in most > cases? We could. Like we dump MEM_REF as * in some cases. The question is still why fix things half-way if a complete solution is already there? Btw, VIEW_CONVERT dumping uses () instead of [], that I used [] when I introduced MEM_REF was probably a mistake... Is it just the parens kind you dislike? Richard. > > Jakub
On 6/3/19 4:34 AM, Richard Biener wrote: > On Mon, Jun 3, 2019 at 10:57 AM Jakub Jelinek <jakub@redhat.com> wrote: >> >> On Mon, Jun 03, 2019 at 10:36:42AM +0200, Richard Biener wrote: >>>> To avoid this confusion the attached patch adds to the dump >>>> a cast to the MEM_REF type for accesses whose size is not equal >>>> to the size of the operand (when the sizes are the same no new >>>> cast is prepended). The effect is that with store merging in >>>> effect, the dump for the above becomes >>>> >>>> MEM[(short int *)(char *)&a] = 1; >>> >>> I think this is confusing syntax. Iff you absolutely refuse to >>> make the -gimple dump the default for MEM_REF and you insist >>> on fixing this issue then please follow how we dump VIEW_CONVERT_EXPR >>> which is the only other tree code we dump the access type, thus >> >> I must say I prefer the current MEM[ over the -gimple for human readable >> dumps. > > Sure, but then why ask for all information to be present when in the cases > you are curious you can look at -gimple dumps? A similar thing I've > hacked the pretty printer locally for debugging in the past is alignment info. > >>> MEM<short int *>[(char *)&a] = 1; >> >> Wouldn't that be >> MEM<short int>[(char *)&a] instead? > > Err, yes. > >> Couldn't we do it only if the TREE_TYPE (TREE_TYPE (TREE_OPERAND (mem, 1))) >> is not compatible with TREE_TYPE (mem), so keep what we were doing in most >> cases? > > We could. Like we dump MEM_REF as * in some cases. > > The question is still why fix things half-way if a complete solution > is already there? Because it restores the important detail for those of us who are accustomed to the "legacy" format. That's without a doubt the majority of users. Note that godbolt.org only exposes the classic dumps and doesn't make it possible to select the -gimple form. Those with a preference for the -gimple syntax are presumably already using the -gimple dumps so they shouldn't be bothered by a change to the legacy format. But those with a preference for the traditional syntax will not appreciate having the syntax changed. Scripts that parse those dumps (like GCC's own test harness) have a reasonable chance of continuing to be able to parse the syntax even with the additional cast. They will certainly not be able to parse it if it changes to MEM<T>(...)). But if you refuse to accept the patch as is and insist on the syntax with the pointy brackets please let me know. I think it's more important get the size of the access restored than the details of the syntax so I'm willing to spend the time to adjust the fix, even at the risk of breaking scripts and making some users unhappy. Martin > > Btw, VIEW_CONVERT dumping uses () instead of [], that I used > [] when I introduced MEM_REF was probably a mistake... > Is it just the parens kind you dislike? > > Richard. > >> >> Jakub
On Mon, Jun 3, 2019 at 5:13 PM Martin Sebor <msebor@gmail.com> wrote: > > On 6/3/19 4:34 AM, Richard Biener wrote: > > On Mon, Jun 3, 2019 at 10:57 AM Jakub Jelinek <jakub@redhat.com> wrote: > >> > >> On Mon, Jun 03, 2019 at 10:36:42AM +0200, Richard Biener wrote: > >>>> To avoid this confusion the attached patch adds to the dump > >>>> a cast to the MEM_REF type for accesses whose size is not equal > >>>> to the size of the operand (when the sizes are the same no new > >>>> cast is prepended). The effect is that with store merging in > >>>> effect, the dump for the above becomes > >>>> > >>>> MEM[(short int *)(char *)&a] = 1; > >>> > >>> I think this is confusing syntax. Iff you absolutely refuse to > >>> make the -gimple dump the default for MEM_REF and you insist > >>> on fixing this issue then please follow how we dump VIEW_CONVERT_EXPR > >>> which is the only other tree code we dump the access type, thus > >> > >> I must say I prefer the current MEM[ over the -gimple for human readable > >> dumps. > > > > Sure, but then why ask for all information to be present when in the cases > > you are curious you can look at -gimple dumps? A similar thing I've > > hacked the pretty printer locally for debugging in the past is alignment info. > > > >>> MEM<short int *>[(char *)&a] = 1; > >> > >> Wouldn't that be > >> MEM<short int>[(char *)&a] instead? > > > > Err, yes. > > > >> Couldn't we do it only if the TREE_TYPE (TREE_TYPE (TREE_OPERAND (mem, 1))) > >> is not compatible with TREE_TYPE (mem), so keep what we were doing in most > >> cases? > > > > We could. Like we dump MEM_REF as * in some cases. > > > > The question is still why fix things half-way if a complete solution > > is already there? > > Because it restores the important detail for those of us who > are accustomed to the "legacy" format. That's without a doubt > the majority of users. Note that godbolt.org only exposes > the classic dumps and doesn't make it possible to select > the -gimple form. > > Those with a preference for the -gimple syntax are presumably > already using the -gimple dumps so they shouldn't be bothered > by a change to the legacy format. > > But those with a preference for the traditional syntax will > not appreciate having the syntax changed. Scripts that parse > those dumps (like GCC's own test harness) have a reasonable > chance of continuing to be able to parse the syntax even with > the additional cast. They will certainly not be able to parse > it if it changes to MEM<T>(...)). > > But if you refuse to accept the patch as is and insist on > the syntax with the pointy brackets please let me know. I > think it's more important get the size of the access restored > than the details of the syntax so I'm willing to spend the time > to adjust the fix, even at the risk of breaking scripts and > making some users unhappy. I think introducing inconsistencies (and I find two "casts" confusing as well) with existing VIEW_CONVERT_EXPR dumping isn't good. So yes, I'd rather prefer MEM <access-type> [(alias-pointer-type) ptr] note access-type isn't a pointer type to the access type. I can definitely live with this incremental but consistent change. Also consider eliding access-type dumping as Jakub suggested (when equal to *alias-pointer-type). As said in the PR dump format changes have the chance to make testcases testing for sth _not_ to appear no longer testing what they want to test for (one reason those testcases are broken). A quick grep for MEM on a scan-*-dump-not might reveal candidates that could need a second look. Note that it was pure laziness on my side that I didn't change the "legacy" format for the way the GIMPLE FE likes it :/ And I feel sorry about that. Richard. > Martin > > > > > Btw, VIEW_CONVERT dumping uses () instead of [], that I used > > [] when I introduced MEM_REF was probably a mistake... > > Is it just the parens kind you dislike? > > > > Richard. > > > >> > >> Jakub >
The attached update adds the MEM_REF type in pointy braces like in the -gimple format, but without the access size: MEM <short int> [(char *)&a] = 1; Martin On 6/4/19 4:57 AM, Richard Biener wrote: > On Mon, Jun 3, 2019 at 5:13 PM Martin Sebor <msebor@gmail.com> wrote: >> >> On 6/3/19 4:34 AM, Richard Biener wrote: >>> On Mon, Jun 3, 2019 at 10:57 AM Jakub Jelinek <jakub@redhat.com> wrote: >>>> >>>> On Mon, Jun 03, 2019 at 10:36:42AM +0200, Richard Biener wrote: >>>>>> To avoid this confusion the attached patch adds to the dump >>>>>> a cast to the MEM_REF type for accesses whose size is not equal >>>>>> to the size of the operand (when the sizes are the same no new >>>>>> cast is prepended). The effect is that with store merging in >>>>>> effect, the dump for the above becomes >>>>>> >>>>>> MEM[(short int *)(char *)&a] = 1; >>>>> >>>>> I think this is confusing syntax. Iff you absolutely refuse to >>>>> make the -gimple dump the default for MEM_REF and you insist >>>>> on fixing this issue then please follow how we dump VIEW_CONVERT_EXPR >>>>> which is the only other tree code we dump the access type, thus >>>> >>>> I must say I prefer the current MEM[ over the -gimple for human readable >>>> dumps. >>> >>> Sure, but then why ask for all information to be present when in the cases >>> you are curious you can look at -gimple dumps? A similar thing I've >>> hacked the pretty printer locally for debugging in the past is alignment info. >>> >>>>> MEM<short int *>[(char *)&a] = 1; >>>> >>>> Wouldn't that be >>>> MEM<short int>[(char *)&a] instead? >>> >>> Err, yes. >>> >>>> Couldn't we do it only if the TREE_TYPE (TREE_TYPE (TREE_OPERAND (mem, 1))) >>>> is not compatible with TREE_TYPE (mem), so keep what we were doing in most >>>> cases? >>> >>> We could. Like we dump MEM_REF as * in some cases. >>> >>> The question is still why fix things half-way if a complete solution >>> is already there? >> >> Because it restores the important detail for those of us who >> are accustomed to the "legacy" format. That's without a doubt >> the majority of users. Note that godbolt.org only exposes >> the classic dumps and doesn't make it possible to select >> the -gimple form. >> >> Those with a preference for the -gimple syntax are presumably >> already using the -gimple dumps so they shouldn't be bothered >> by a change to the legacy format. >> >> But those with a preference for the traditional syntax will >> not appreciate having the syntax changed. Scripts that parse >> those dumps (like GCC's own test harness) have a reasonable >> chance of continuing to be able to parse the syntax even with >> the additional cast. They will certainly not be able to parse >> it if it changes to MEM<T>(...)). >> >> But if you refuse to accept the patch as is and insist on >> the syntax with the pointy brackets please let me know. I >> think it's more important get the size of the access restored >> than the details of the syntax so I'm willing to spend the time >> to adjust the fix, even at the risk of breaking scripts and >> making some users unhappy. > > I think introducing inconsistencies (and I find two "casts" > confusing as well) with existing VIEW_CONVERT_EXPR > dumping isn't good. So yes, I'd rather prefer > > MEM <access-type> [(alias-pointer-type) ptr] > > note access-type isn't a pointer type to the access type. > > I can definitely live with this incremental but consistent change. > Also consider eliding access-type dumping as Jakub suggested > (when equal to *alias-pointer-type). > > As said in the PR dump format changes have the chance > to make testcases testing for sth _not_ to appear no longer > testing what they want to test for (one reason those testcases > are broken). A quick grep for MEM on a scan-*-dump-not > might reveal candidates that could need a second look. > > Note that it was pure laziness on my side that I didn't change > the "legacy" format for the way the GIMPLE FE likes it :/ > And I feel sorry about that. > > Richard. > >> Martin >> >>> >>> Btw, VIEW_CONVERT dumping uses () instead of [], that I used >>> [] when I introduced MEM_REF was probably a mistake... >>> Is it just the parens kind you dislike? >>> >>> Richard. >>> >>>> >>>> Jakub >>
On Mon, Jun 10, 2019 at 01:23:28PM -0600, Martin Sebor wrote: > + else if (integer_zerop (TREE_OPERAND (node, 1)) > + /* Dump the types of INTEGER_CSTs explicitly, for we can't > + infer them and MEM_ATTR caching will share MEM_REFs > + with differently-typed op0s. */ > + && TREE_CODE (TREE_OPERAND (node, 0)) != INTEGER_CST > + /* Released SSA_NAMES have no TREE_TYPE. */ > + && TREE_TYPE (TREE_OPERAND (node, 0)) != NULL_TREE > + /* Same pointer types, but ignoring POINTER_TYPE vs. > + REFERENCE_TYPE. */ > + && (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 0))) > + == TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1)))) > + && (TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 0))) > + == TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 1)))) > + && (TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 0))) > + == TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 1)))) > + /* Same value types ignoring qualifiers. */ > + && (TYPE_MAIN_VARIANT (TREE_TYPE (node)) > + == TYPE_MAIN_VARIANT > + (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1))))) You should be using types_compatible_p rather than type equality, that is all GIMPLE cares about. Jakub
On 6/10/19 1:29 PM, Jakub Jelinek wrote: > On Mon, Jun 10, 2019 at 01:23:28PM -0600, Martin Sebor wrote: >> + else if (integer_zerop (TREE_OPERAND (node, 1)) >> + /* Dump the types of INTEGER_CSTs explicitly, for we can't >> + infer them and MEM_ATTR caching will share MEM_REFs >> + with differently-typed op0s. */ >> + && TREE_CODE (TREE_OPERAND (node, 0)) != INTEGER_CST >> + /* Released SSA_NAMES have no TREE_TYPE. */ >> + && TREE_TYPE (TREE_OPERAND (node, 0)) != NULL_TREE >> + /* Same pointer types, but ignoring POINTER_TYPE vs. >> + REFERENCE_TYPE. */ >> + && (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 0))) >> + == TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1)))) >> + && (TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 0))) >> + == TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 1)))) >> + && (TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 0))) >> + == TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 1)))) >> + /* Same value types ignoring qualifiers. */ >> + && (TYPE_MAIN_VARIANT (TREE_TYPE (node)) >> + == TYPE_MAIN_VARIANT >> + (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1))))) > > You should be using types_compatible_p rather than type equality, that is > all GIMPLE cares about. The code above predates my change, I just factored it out into a separate function; it does make the diff bigger. The code I introduced only adds the <...> if the size of the access differs from the size of the operand. I used type sizes rather than testing their compatibility to minimize the number of tests that might need updating. This is the salient bit: + pp_string (pp, "MEM"); + + tree nodetype = TREE_TYPE (node); + tree op0 = TREE_OPERAND (node, 0); + tree op1 = TREE_OPERAND (node, 1); + tree op1type = TYPE_MAIN_VARIANT (TREE_TYPE (op1)); + + if (!tree_int_cst_equal (TYPE_SIZE (nodetype), + TYPE_SIZE (TREE_TYPE (op1type)))) + { + pp_string (pp, " <"); + /* If the size of the type of the operand is not the same + as the size of the MEM_REF expression include the type + of the latter similar to the TDF_GIMPLE output to make + it clear how many bytes of memory are being accessed. */ + dump_generic_node (pp, nodetype, spc, flags | TDF_SLIM, false); + pp_string (pp, "> "); + } Martin
On Mon, Jun 10, 2019 at 10:37 PM Martin Sebor <msebor@gmail.com> wrote: > > On 6/10/19 1:29 PM, Jakub Jelinek wrote: > > On Mon, Jun 10, 2019 at 01:23:28PM -0600, Martin Sebor wrote: > >> + else if (integer_zerop (TREE_OPERAND (node, 1)) > >> + /* Dump the types of INTEGER_CSTs explicitly, for we can't > >> + infer them and MEM_ATTR caching will share MEM_REFs > >> + with differently-typed op0s. */ > >> + && TREE_CODE (TREE_OPERAND (node, 0)) != INTEGER_CST > >> + /* Released SSA_NAMES have no TREE_TYPE. */ > >> + && TREE_TYPE (TREE_OPERAND (node, 0)) != NULL_TREE > >> + /* Same pointer types, but ignoring POINTER_TYPE vs. > >> + REFERENCE_TYPE. */ > >> + && (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 0))) > >> + == TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1)))) > >> + && (TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 0))) > >> + == TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 1)))) > >> + && (TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 0))) > >> + == TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 1)))) > >> + /* Same value types ignoring qualifiers. */ > >> + && (TYPE_MAIN_VARIANT (TREE_TYPE (node)) > >> + == TYPE_MAIN_VARIANT > >> + (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1))))) > > > > You should be using types_compatible_p rather than type equality, that is > > all GIMPLE cares about. > > The code above predates my change, I just factored it out into > a separate function; it does make the diff bigger. The code > I introduced only adds the <...> if the size of the access > differs from the size of the operand. I used type sizes rather > than testing their compatibility to minimize the number of tests > that might need updating. > > This is the salient bit: > > + pp_string (pp, "MEM"); > + > + tree nodetype = TREE_TYPE (node); > + tree op0 = TREE_OPERAND (node, 0); > + tree op1 = TREE_OPERAND (node, 1); > + tree op1type = TYPE_MAIN_VARIANT (TREE_TYPE (op1)); > + > + if (!tree_int_cst_equal (TYPE_SIZE (nodetype), > + TYPE_SIZE (TREE_TYPE (op1type)))) > + { > + pp_string (pp, " <"); > + /* If the size of the type of the operand is not the same > + as the size of the MEM_REF expression include the type > + of the latter similar to the TDF_GIMPLE output to make > + it clear how many bytes of memory are being accessed. */ > + dump_generic_node (pp, nodetype, spc, flags | TDF_SLIM, false); > + pp_string (pp, "> "); > + } You need to guard against non-constant TYPE_SIZE here (for both involved types) so I suggest you use operand_equal_p (..., 0). If you do that you need to guard against NULL_TREE TYPE_SIZE (tree_int_cst_equal handled that as unequal). OK with such change. Richard. > Martin
Richard Biener <richard.guenther@gmail.com> writes: > On Mon, Jun 10, 2019 at 10:37 PM Martin Sebor <msebor@gmail.com> wrote: >> >> On 6/10/19 1:29 PM, Jakub Jelinek wrote: >> > On Mon, Jun 10, 2019 at 01:23:28PM -0600, Martin Sebor wrote: >> >> + else if (integer_zerop (TREE_OPERAND (node, 1)) >> >> + /* Dump the types of INTEGER_CSTs explicitly, for we can't >> >> + infer them and MEM_ATTR caching will share MEM_REFs >> >> + with differently-typed op0s. */ >> >> + && TREE_CODE (TREE_OPERAND (node, 0)) != INTEGER_CST >> >> + /* Released SSA_NAMES have no TREE_TYPE. */ >> >> + && TREE_TYPE (TREE_OPERAND (node, 0)) != NULL_TREE >> >> + /* Same pointer types, but ignoring POINTER_TYPE vs. >> >> + REFERENCE_TYPE. */ >> >> + && (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 0))) >> >> + == TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1)))) >> >> + && (TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 0))) >> >> + == TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 1)))) >> >> + && (TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 0))) >> >> + == TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 1)))) >> >> + /* Same value types ignoring qualifiers. */ >> >> + && (TYPE_MAIN_VARIANT (TREE_TYPE (node)) >> >> + == TYPE_MAIN_VARIANT >> >> + (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1))))) >> > >> > You should be using types_compatible_p rather than type equality, that is >> > all GIMPLE cares about. >> >> The code above predates my change, I just factored it out into >> a separate function; it does make the diff bigger. The code >> I introduced only adds the <...> if the size of the access >> differs from the size of the operand. I used type sizes rather >> than testing their compatibility to minimize the number of tests >> that might need updating. >> >> This is the salient bit: >> >> + pp_string (pp, "MEM"); >> + >> + tree nodetype = TREE_TYPE (node); >> + tree op0 = TREE_OPERAND (node, 0); >> + tree op1 = TREE_OPERAND (node, 1); >> + tree op1type = TYPE_MAIN_VARIANT (TREE_TYPE (op1)); >> + >> + if (!tree_int_cst_equal (TYPE_SIZE (nodetype), >> + TYPE_SIZE (TREE_TYPE (op1type)))) >> + { >> + pp_string (pp, " <"); >> + /* If the size of the type of the operand is not the same >> + as the size of the MEM_REF expression include the type >> + of the latter similar to the TDF_GIMPLE output to make >> + it clear how many bytes of memory are being accessed. */ >> + dump_generic_node (pp, nodetype, spc, flags | TDF_SLIM, false); >> + pp_string (pp, "> "); >> + } > > You need to guard against non-constant TYPE_SIZE here (for both > involved types) so I suggest you use operand_equal_p (..., 0). If you > do that you need to guard against NULL_TREE TYPE_SIZE > (tree_int_cst_equal handled that as unequal). > > OK with such change. > Richard. The testsuite part cannot have been tested very thoroughly: for one, this snippet diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c --- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c @@ -59,4 +59,5 @@ void foo(int prec, bar (&info); } -/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct printf_info \\*\\)&info \\+ \[0-9\]+B\\\] = {}" 1 "dse1" } } */ +/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct printf_info \\*\\)&info \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { ! store_merge } } } } + { dg-final { scan-tree-dump-times "MEM <char[4]> \\\[\\(struct printf_info \\*\\)&info \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { store_merge } } } } */ breaks gcc.sum creation with dg-extract-result.py (the default) completely: gcc.log shows spawn -ignore SIGHUP /var/gcc/regression/trunk/11-gcc/build/gcc/xgcc -B/var/gcc/regression/trunk/11-gcc/build/gcc/ /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers -fdiagnostics-color=never -O2 -fdump-tree-dse1 -S -o ssa-dse-24.s^M PASS: gcc.dg/tree-ssa/ssa-dse-24.c (test for excess errors) ERROR: (DejaGnu) proc "4" does not exist. The error code is NONE The info on the error is: invalid command name "4" while executing "::tcl_unknown 4" ("uplevel" body line 1) invoked from within "uplevel 1 ::tcl_unknown $args" due to the unquoted [4]. Even with that fixed, I see many failures: +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++14 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++17 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++98 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" +FAIL: g++.dg/tree-ssa/ssa-dse-1.C scan-tree-dump-times dse1 "MEM <char\\\\[176]> \\\\[\\\\(struct FixBuf \\\\*\\\\)&<retval> \\\\+ [0-9]+B\\\\] = {}" 1 on 32 and 64-bit i386-pc-solaris2.11 (and i686-pc-linux-gnu), +FAIL: g++.dg/tree-ssa/pr19807.C -std=gnu++14 scan-tree-dump-times optimized "&MEM\\\\[\\\\(void .\\\\)&a \\\\+ 8B\\\\]" 3 +FAIL: g++.dg/tree-ssa/pr19807.C -std=gnu++17 scan-tree-dump-times optimized "&MEM\\\\[\\\\(void .\\\\)&a \\\\+ 8B\\\\]" 3 +FAIL: g++.dg/tree-ssa/pr19807.C -std=gnu++98 scan-tree-dump-times optimized "&MEM\\\\[\\\\(void .\\\\)&a \\\\+ 8B\\\\]" 3 +FAIL: gcc.dg/tree-prof/stringop-2.c scan-tree-dump optimized "MEM\\\\[\\\\(void .\\\\)&a\\\\] = 168430090" +FAIL: gcc.dg/tree-ssa/pr30375.c scan-tree-dump-times dse1 "MEM\\\\[\\\\(struct _s \\\\*\\\\)&signInfo \\\\+ [0-9]+B\\\\] = {}" 1 +FAIL: gcc.dg/tree-ssa/slsr-27.c scan-tree-dump-times dom3 "MEM\\\\[\\\\(struct x \\\\*\\\\)[^\\r\\n]*_\\\\d+" 3 +FAIL: gcc.dg/tree-ssa/slsr-28.c scan-tree-dump-times dom3 "MEM\\\\[\\\\(struct x \\\\*\\\\)[^\\r\\n]*_\\\\d+" 9 +FAIL: gcc.dg/tree-ssa/slsr-29.c scan-tree-dump-times dom3 "MEM\\\\[\\\\(struct x \\\\*\\\\)[^\\r\\n]*_\\\\d+" 9 +FAIL: gcc.dg/tree-ssa/ssa-dse-24.c scan-tree-dump-times dse1 "MEM\\\\[\\\\(struct printf_info \\\\*\\\\)&info \\\\+ [0-9]+B\\\\] = {}" 1 on 32 and 64-bit sparc-sun-solaris2.11 (the first of those also on m68k-unknown-linux-gnu). I haven't even started looking what's wrong (quoting problems in the REs or other issues). Rainer
On 6/12/19 3:33 PM, Rainer Orth wrote: > Richard Biener <richard.guenther@gmail.com> writes: > >> On Mon, Jun 10, 2019 at 10:37 PM Martin Sebor <msebor@gmail.com> wrote: >>> >>> On 6/10/19 1:29 PM, Jakub Jelinek wrote: >>>> On Mon, Jun 10, 2019 at 01:23:28PM -0600, Martin Sebor wrote: >>>>> + else if (integer_zerop (TREE_OPERAND (node, 1)) >>>>> + /* Dump the types of INTEGER_CSTs explicitly, for we can't >>>>> + infer them and MEM_ATTR caching will share MEM_REFs >>>>> + with differently-typed op0s. */ >>>>> + && TREE_CODE (TREE_OPERAND (node, 0)) != INTEGER_CST >>>>> + /* Released SSA_NAMES have no TREE_TYPE. */ >>>>> + && TREE_TYPE (TREE_OPERAND (node, 0)) != NULL_TREE >>>>> + /* Same pointer types, but ignoring POINTER_TYPE vs. >>>>> + REFERENCE_TYPE. */ >>>>> + && (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 0))) >>>>> + == TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1)))) >>>>> + && (TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 0))) >>>>> + == TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 1)))) >>>>> + && (TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 0))) >>>>> + == TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 1)))) >>>>> + /* Same value types ignoring qualifiers. */ >>>>> + && (TYPE_MAIN_VARIANT (TREE_TYPE (node)) >>>>> + == TYPE_MAIN_VARIANT >>>>> + (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1))))) >>>> >>>> You should be using types_compatible_p rather than type equality, that is >>>> all GIMPLE cares about. >>> >>> The code above predates my change, I just factored it out into >>> a separate function; it does make the diff bigger. The code >>> I introduced only adds the <...> if the size of the access >>> differs from the size of the operand. I used type sizes rather >>> than testing their compatibility to minimize the number of tests >>> that might need updating. >>> >>> This is the salient bit: >>> >>> + pp_string (pp, "MEM"); >>> + >>> + tree nodetype = TREE_TYPE (node); >>> + tree op0 = TREE_OPERAND (node, 0); >>> + tree op1 = TREE_OPERAND (node, 1); >>> + tree op1type = TYPE_MAIN_VARIANT (TREE_TYPE (op1)); >>> + >>> + if (!tree_int_cst_equal (TYPE_SIZE (nodetype), >>> + TYPE_SIZE (TREE_TYPE (op1type)))) >>> + { >>> + pp_string (pp, " <"); >>> + /* If the size of the type of the operand is not the same >>> + as the size of the MEM_REF expression include the type >>> + of the latter similar to the TDF_GIMPLE output to make >>> + it clear how many bytes of memory are being accessed. */ >>> + dump_generic_node (pp, nodetype, spc, flags | TDF_SLIM, false); >>> + pp_string (pp, "> "); >>> + } >> >> You need to guard against non-constant TYPE_SIZE here (for both >> involved types) so I suggest you use operand_equal_p (..., 0). If you >> do that you need to guard against NULL_TREE TYPE_SIZE >> (tree_int_cst_equal handled that as unequal). >> >> OK with such change. >> Richard. > > The testsuite part cannot have been tested very thoroughly: for one, > this snippet Please try r272218. Martin > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c > --- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c > +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c > @@ -59,4 +59,5 @@ void foo(int prec, > bar (&info); > } > > -/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct printf_info \\*\\)&info \\+ \[0-9\]+B\\\] = {}" 1 "dse1" } } */ > +/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct printf_info \\*\\)&info \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { ! store_merge } } } } > + { dg-final { scan-tree-dump-times "MEM <char[4]> \\\[\\(struct printf_info \\*\\)&info \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { store_merge } } } } */ > > > breaks gcc.sum creation with dg-extract-result.py (the default) > completely: gcc.log shows > > spawn -ignore SIGHUP /var/gcc/regression/trunk/11-gcc/build/gcc/xgcc -B/var/gcc/regression/trunk/11-gcc/build/gcc/ /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers -fdiagnostics-color=never -O2 -fdump-tree-dse1 -S -o ssa-dse-24.s^M > PASS: gcc.dg/tree-ssa/ssa-dse-24.c (test for excess errors) > ERROR: (DejaGnu) proc "4" does not exist. > The error code is NONE > The info on the error is: > invalid command name "4" > while executing > "::tcl_unknown 4" > ("uplevel" body line 1) > invoked from within > "uplevel 1 ::tcl_unknown $args" > > due to the unquoted [4]. > > Even with that fixed, I see many failures: > > +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++14 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" > +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++17 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" > +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++98 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" > +FAIL: g++.dg/tree-ssa/ssa-dse-1.C scan-tree-dump-times dse1 "MEM <char\\\\[176]> \\\\[\\\\(struct FixBuf \\\\*\\\\)&<retval> \\\\+ [0-9]+B\\\\] = {}" 1 > > on 32 and 64-bit i386-pc-solaris2.11 (and i686-pc-linux-gnu), > > +FAIL: g++.dg/tree-ssa/pr19807.C -std=gnu++14 scan-tree-dump-times optimized "&MEM\\\\[\\\\(void .\\\\)&a \\\\+ 8B\\\\]" 3 > +FAIL: g++.dg/tree-ssa/pr19807.C -std=gnu++17 scan-tree-dump-times optimized "&MEM\\\\[\\\\(void .\\\\)&a \\\\+ 8B\\\\]" 3 > +FAIL: g++.dg/tree-ssa/pr19807.C -std=gnu++98 scan-tree-dump-times optimized "&MEM\\\\[\\\\(void .\\\\)&a \\\\+ 8B\\\\]" 3 > > +FAIL: gcc.dg/tree-prof/stringop-2.c scan-tree-dump optimized "MEM\\\\[\\\\(void .\\\\)&a\\\\] = 168430090" > +FAIL: gcc.dg/tree-ssa/pr30375.c scan-tree-dump-times dse1 "MEM\\\\[\\\\(struct _s \\\\*\\\\)&signInfo \\\\+ [0-9]+B\\\\] = {}" 1 > +FAIL: gcc.dg/tree-ssa/slsr-27.c scan-tree-dump-times dom3 "MEM\\\\[\\\\(struct x \\\\*\\\\)[^\\r\\n]*_\\\\d+" 3 > +FAIL: gcc.dg/tree-ssa/slsr-28.c scan-tree-dump-times dom3 "MEM\\\\[\\\\(struct x \\\\*\\\\)[^\\r\\n]*_\\\\d+" 9 > +FAIL: gcc.dg/tree-ssa/slsr-29.c scan-tree-dump-times dom3 "MEM\\\\[\\\\(struct x \\\\*\\\\)[^\\r\\n]*_\\\\d+" 9 > +FAIL: gcc.dg/tree-ssa/ssa-dse-24.c scan-tree-dump-times dse1 "MEM\\\\[\\\\(struct printf_info \\\\*\\\\)&info \\\\+ [0-9]+B\\\\] = {}" 1 > > on 32 and 64-bit sparc-sun-solaris2.11 (the first of those also on > m68k-unknown-linux-gnu). > > I haven't even started looking what's wrong (quoting problems in the REs > or other issues).
Hi Martin, >> The testsuite part cannot have been tested very thoroughly: for one, >> this snippet > > Please try r272218. this cured a couple of issues, like this one: >> ERROR: (DejaGnu) proc "4" does not exist. However... >> Even with that fixed, I see many failures: >> >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++14 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++17 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++98 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" >> +FAIL: g++.dg/tree-ssa/ssa-dse-1.C scan-tree-dump-times dse1 "MEM <char\\\\[176]> \\\\[\\\\(struct FixBuf \\\\*\\\\)&<retval> \\\\+ [0-9]+B\\\\] = {}" 1 >> >> on 32 and 64-bit i386-pc-solaris2.11 (and i686-pc-linux-gnu), these failures remain... >> +FAIL: g++.dg/tree-ssa/pr19807.C -std=gnu++14 scan-tree-dump-times optimized "&MEM\\\\[\\\\(void .\\\\)&a \\\\+ 8B\\\\]" 3 >> +FAIL: g++.dg/tree-ssa/pr19807.C -std=gnu++17 scan-tree-dump-times optimized "&MEM\\\\[\\\\(void .\\\\)&a \\\\+ 8B\\\\]" 3 >> +FAIL: g++.dg/tree-ssa/pr19807.C -std=gnu++98 scan-tree-dump-times optimized "&MEM\\\\[\\\\(void .\\\\)&a \\\\+ 8B\\\\]" 3 as does this one... >> +FAIL: gcc.dg/tree-prof/stringop-2.c scan-tree-dump optimized "MEM\\\\[\\\\(void .\\\\)&a\\\\] = 168430090" and this one... >> +FAIL: gcc.dg/tree-ssa/pr30375.c scan-tree-dump-times dse1 "MEM\\\\[\\\\(struct _s \\\\*\\\\)&signInfo \\\\+ [0-9]+B\\\\] = {}" 1 >> +FAIL: gcc.dg/tree-ssa/slsr-27.c scan-tree-dump-times dom3 "MEM\\\\[\\\\(struct x \\\\*\\\\)[^\\r\\n]*_\\\\d+" 3 >> +FAIL: gcc.dg/tree-ssa/slsr-28.c scan-tree-dump-times dom3 "MEM\\\\[\\\\(struct x \\\\*\\\\)[^\\r\\n]*_\\\\d+" 9 >> +FAIL: gcc.dg/tree-ssa/slsr-29.c scan-tree-dump-times dom3 "MEM\\\\[\\\\(struct x \\\\*\\\\)[^\\r\\n]*_\\\\d+" 9 >> +FAIL: gcc.dg/tree-ssa/ssa-dse-24.c scan-tree-dump-times dse1 "MEM\\\\[\\\\(struct printf_info \\\\*\\\\)&info \\\\+ [0-9]+B\\\\] = {}" 1 >> >> on 32 and 64-bit sparc-sun-solaris2.11 (the first of those also on >> m68k-unknown-linux-gnu). while this group is gone now. Rainer
On Thu, Jun 13, 2019 at 10:53:55AM +0200, Rainer Orth wrote: > >> Even with that fixed, I see many failures: > >> > >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++14 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" > >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++17 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" > >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++98 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" > >> +FAIL: g++.dg/tree-ssa/ssa-dse-1.C scan-tree-dump-times dse1 "MEM <char\\\\[176]> \\\\[\\\\(struct FixBuf \\\\*\\\\)&<retval> \\\\+ [0-9]+B\\\\] = {}" 1 > >> > >> on 32 and 64-bit i386-pc-solaris2.11 (and i686-pc-linux-gnu), > > these failures remain... On i686-linux I can reproduce just the above ones. The following should fix it. As we don't match exact offset on the MEM because it varries between different architectures (24 bytes on with -m64, 28 bytes with -m32), we shouldn't match the store size either, as it is 200 - that offset, so 176 or 172 etc. Tested on x86_64-linux, -m32/-m64, ok for trunk? 2019-06-13 Jakub Jelinek <jakub@redhat.com> * g++.dg/tree-ssa/ssa-dse-1.C: Don't match exact number of chars of = {} store. * g++.dg/tree-ssa/pr31146.C: Change -fdump-tree-forwprop to -fdump-tree-forwprop1 in dg-options. Expect <int[5]> in MEM. --- gcc/testsuite/g++.dg/tree-ssa/ssa-dse-1.C.jj 2019-06-13 00:35:49.654840275 +0200 +++ gcc/testsuite/g++.dg/tree-ssa/ssa-dse-1.C 2019-06-13 12:40:14.492568336 +0200 @@ -98,4 +98,4 @@ int main() /* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { ! store_merge } } } } - { dg-final { scan-tree-dump-times "MEM <char\\\[176]> \\\[\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { store_merge } } } } */ + { dg-final { scan-tree-dump-times "MEM <char\\\[\[0-9\]+]> \\\[\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { store_merge } } } } */ --- gcc/testsuite/g++.dg/tree-ssa/pr31146.C.jj 2015-05-29 15:04:33.039803414 +0200 +++ gcc/testsuite/g++.dg/tree-ssa/pr31146.C 2019-06-13 12:24:15.895576933 +0200 @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O -fdump-tree-forwprop" } */ +/* { dg-options "-O -fdump-tree-forwprop1" } */ /* We should be able to optimize this to i[j] = 1 during early optimizations. */ @@ -12,4 +12,4 @@ void foo (int j) *q = 1; } -/* { dg-final { scan-tree-dump "MEM\\\[.*&i\\\]\\\[j.*\\\] =.* 1;" "forwprop1" } } */ +/* { dg-final { scan-tree-dump "MEM <int\\\[5\\\]> \\\[.*&i\\\]\\\[j.*\\\] =.* 1;" "forwprop1" } } */ Jakub
On Thu, Jun 13, 2019 at 12:45 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Jun 13, 2019 at 10:53:55AM +0200, Rainer Orth wrote: > > >> Even with that fixed, I see many failures: > > >> > > >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++14 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" > > >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++17 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" > > >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++98 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" > > >> +FAIL: g++.dg/tree-ssa/ssa-dse-1.C scan-tree-dump-times dse1 "MEM <char\\\\[176]> \\\\[\\\\(struct FixBuf \\\\*\\\\)&<retval> \\\\+ [0-9]+B\\\\] = {}" 1 > > >> > > >> on 32 and 64-bit i386-pc-solaris2.11 (and i686-pc-linux-gnu), > > > > these failures remain... > > On i686-linux I can reproduce just the above ones. > The following should fix it. As we don't match exact offset on the MEM > because it varries between different architectures (24 bytes on with -m64, > 28 bytes with -m32), we shouldn't match the store size either, as it is > 200 - that offset, so 176 or 172 etc. > > Tested on x86_64-linux, -m32/-m64, ok for trunk? OK. Richard. > 2019-06-13 Jakub Jelinek <jakub@redhat.com> > > * g++.dg/tree-ssa/ssa-dse-1.C: Don't match exact number of chars of > = {} store. > * g++.dg/tree-ssa/pr31146.C: Change -fdump-tree-forwprop to > -fdump-tree-forwprop1 in dg-options. Expect <int[5]> in MEM. > > --- gcc/testsuite/g++.dg/tree-ssa/ssa-dse-1.C.jj 2019-06-13 00:35:49.654840275 +0200 > +++ gcc/testsuite/g++.dg/tree-ssa/ssa-dse-1.C 2019-06-13 12:40:14.492568336 +0200 > @@ -98,4 +98,4 @@ int main() > > > /* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { ! store_merge } } } } > - { dg-final { scan-tree-dump-times "MEM <char\\\[176]> \\\[\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { store_merge } } } } */ > + { dg-final { scan-tree-dump-times "MEM <char\\\[\[0-9\]+]> \\\[\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { store_merge } } } } */ > --- gcc/testsuite/g++.dg/tree-ssa/pr31146.C.jj 2015-05-29 15:04:33.039803414 +0200 > +++ gcc/testsuite/g++.dg/tree-ssa/pr31146.C 2019-06-13 12:24:15.895576933 +0200 > @@ -1,5 +1,5 @@ > /* { dg-do compile } */ > -/* { dg-options "-O -fdump-tree-forwprop" } */ > +/* { dg-options "-O -fdump-tree-forwprop1" } */ > > /* We should be able to optimize this to i[j] = 1 during > early optimizations. */ > @@ -12,4 +12,4 @@ void foo (int j) > *q = 1; > } > > -/* { dg-final { scan-tree-dump "MEM\\\[.*&i\\\]\\\[j.*\\\] =.* 1;" "forwprop1" } } */ > +/* { dg-final { scan-tree-dump "MEM <int\\\[5\\\]> \\\[.*&i\\\]\\\[j.*\\\] =.* 1;" "forwprop1" } } */ > > > Jakub
Hi Jakub, > On Thu, Jun 13, 2019 at 10:53:55AM +0200, Rainer Orth wrote: >> >> Even with that fixed, I see many failures: >> >> >> >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++14 scan-tree-dump forwprop1 >> >> "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" >> >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++17 scan-tree-dump forwprop1 >> >> "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" >> >> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++98 scan-tree-dump forwprop1 >> >> "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" >> >> +FAIL: g++.dg/tree-ssa/ssa-dse-1.C scan-tree-dump-times dse1 "MEM >> >> <char\\\\[176]> \\\\[\\\\(struct FixBuf \\\\*\\\\)&<retval> \\\\+ >> >> [0-9]+B\\\\] = {}" 1 >> >> >> >> on 32 and 64-bit i386-pc-solaris2.11 (and i686-pc-linux-gnu), >> >> these failures remain... > > On i686-linux I can reproduce just the above ones. right: I'm seeing the rest on sparc-sun-solaris2.11 only (and some of those also on a couple of other targets according to gcc-testresults). Rainer
On 6/13/19 4:44 AM, Jakub Jelinek wrote: > On Thu, Jun 13, 2019 at 10:53:55AM +0200, Rainer Orth wrote: >>>> Even with that fixed, I see many failures: >>>> >>>> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++14 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" >>>> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++17 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" >>>> +FAIL: g++.dg/tree-ssa/pr31146.C -std=gnu++98 scan-tree-dump forwprop1 "MEM\\\\[.*&i\\\\]\\\\[j.*\\\\] =.* 1;" >>>> +FAIL: g++.dg/tree-ssa/ssa-dse-1.C scan-tree-dump-times dse1 "MEM <char\\\\[176]> \\\\[\\\\(struct FixBuf \\\\*\\\\)&<retval> \\\\+ [0-9]+B\\\\] = {}" 1 >>>> >>>> on 32 and 64-bit i386-pc-solaris2.11 (and i686-pc-linux-gnu), >> >> these failures remain... > > On i686-linux I can reproduce just the above ones. > The following should fix it. As we don't match exact offset on the MEM > because it varries between different architectures (24 bytes on with -m64, > 28 bytes with -m32), we shouldn't match the store size either, as it is > 200 - that offset, so 176 or 172 etc. > > Tested on x86_64-linux, -m32/-m64, ok for trunk? > > 2019-06-13 Jakub Jelinek <jakub@redhat.com> > > * g++.dg/tree-ssa/ssa-dse-1.C: Don't match exact number of chars of > = {} store. > * g++.dg/tree-ssa/pr31146.C: Change -fdump-tree-forwprop to > -fdump-tree-forwprop1 in dg-options. Expect <int[5]> in MEM. > > --- gcc/testsuite/g++.dg/tree-ssa/ssa-dse-1.C.jj 2019-06-13 00:35:49.654840275 +0200 > +++ gcc/testsuite/g++.dg/tree-ssa/ssa-dse-1.C 2019-06-13 12:40:14.492568336 +0200 > @@ -98,4 +98,4 @@ int main() > > > /* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { ! store_merge } } } } > - { dg-final { scan-tree-dump-times "MEM <char\\\[176]> \\\[\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { store_merge } } } } */ > + { dg-final { scan-tree-dump-times "MEM <char\\\[\[0-9\]+]> \\\[\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { store_merge } } } } */ > --- gcc/testsuite/g++.dg/tree-ssa/pr31146.C.jj 2015-05-29 15:04:33.039803414 +0200 > +++ gcc/testsuite/g++.dg/tree-ssa/pr31146.C 2019-06-13 12:24:15.895576933 +0200 > @@ -1,5 +1,5 @@ > /* { dg-do compile } */ > -/* { dg-options "-O -fdump-tree-forwprop" } */ > +/* { dg-options "-O -fdump-tree-forwprop1" } */ > > /* We should be able to optimize this to i[j] = 1 during > early optimizations. */ > @@ -12,4 +12,4 @@ void foo (int j) > *q = 1; > } > > -/* { dg-final { scan-tree-dump "MEM\\\[.*&i\\\]\\\[j.*\\\] =.* 1;" "forwprop1" } } */ > +/* { dg-final { scan-tree-dump "MEM <int\\\[5\\\]> \\\[.*&i\\\]\\\[j.*\\\] =.* 1;" "forwprop1" } } */ Thanks for cleaning this up. The size of the access above doesn't look right. The test is: int i[5]; void foo (int j) { void *p = &i[j]; int *q = (int *)p; *q = 1; } and the MEM_REF is for the assignment *q = 1. It assigns a single int, not the whole array. The -gimple output looks pretty much the same: __MEM <int[5]> ((int *)&i)[j_1(D)] = 1; I expected it to mention the size of the access, e.g., like this: __MEM <int> ((int *)&i)[j_1(D)] = 1; It was the entire goal of the change I made to be able to be able to see the size of the access so it doesn't look like I got it right and some more tweaking is necessary. Which also raises the question: what is the purpose of the MEM_REF type if not to encode the size/alignment of the access? Martin
On Thu, Jun 13, 2019 at 09:30:37AM -0600, Martin Sebor wrote:
> The size of the access above doesn't look right. The test is:
It is correct. You have MEM <int[5]> [(int *)&i], which is
the same thing as i itself, and on this you apply an ARRAY_REF,
which is printed after it, with index j_1(D). ARRAY_REF is applied on
arrays and the result type is the array element type, so int in this case.
Jakub
On 6/13/19 9:34 AM, Jakub Jelinek wrote: > On Thu, Jun 13, 2019 at 09:30:37AM -0600, Martin Sebor wrote: >> The size of the access above doesn't look right. The test is: > > It is correct. You have MEM <int[5]> [(int *)&i], which is > the same thing as i itself, and on this you apply an ARRAY_REF, > which is printed after it, with index j_1(D). ARRAY_REF is applied on > arrays and the result type is the array element type, so int in this case. Aah, it's two REFs in one. I misread the array index as being a part of the MEM_REF operand, like this: MEM <int[5]> [((int *)&i)[j_1(D)]] = 1; I guess I've never noticed this before. Why is the whole thing not simplified to an ARRAY_REF? i[j_2(D)] = 1; Martin
On Thu, Jun 13, 2019 at 09:50:16AM -0600, Martin Sebor wrote: > On 6/13/19 9:34 AM, Jakub Jelinek wrote: > > On Thu, Jun 13, 2019 at 09:30:37AM -0600, Martin Sebor wrote: > > > The size of the access above doesn't look right. The test is: > > > > It is correct. You have MEM <int[5]> [(int *)&i], which is > > the same thing as i itself, and on this you apply an ARRAY_REF, > > which is printed after it, with index j_1(D). ARRAY_REF is applied on > > arrays and the result type is the array element type, so int in this case. > > Aah, it's two REFs in one. I misread the array index as being > a part of the MEM_REF operand, like this: > > MEM <int[5]> [((int *)&i)[j_1(D)]] = 1; > > I guess I've never noticed this before. Why is the whole thing > not simplified to an ARRAY_REF? > > i[j_2(D)] = 1; No idea in this case, though of course there can be other cases e.g. where the MEM_REF has different number of elements, different element type etc. from the underlying variable or where the MEM_REF first operand is not address, but pointer. Jakub
On Thu, Jun 13, 2019 at 5:54 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Jun 13, 2019 at 09:50:16AM -0600, Martin Sebor wrote: > > On 6/13/19 9:34 AM, Jakub Jelinek wrote: > > > On Thu, Jun 13, 2019 at 09:30:37AM -0600, Martin Sebor wrote: > > > > The size of the access above doesn't look right. The test is: > > > > > > It is correct. You have MEM <int[5]> [(int *)&i], which is > > > the same thing as i itself, and on this you apply an ARRAY_REF, > > > which is printed after it, with index j_1(D). ARRAY_REF is applied on > > > arrays and the result type is the array element type, so int in this case. > > > > Aah, it's two REFs in one. I misread the array index as being > > a part of the MEM_REF operand, like this: > > > > MEM <int[5]> [((int *)&i)[j_1(D)]] = 1; > > > > I guess I've never noticed this before. Why is the whole thing > > not simplified to an ARRAY_REF? > > > > i[j_2(D)] = 1; > > No idea in this case, though of course there can be other cases e.g. > where the MEM_REF has different number of elements, different element type > etc. from the underlying variable or where the MEM_REF first operand is not > address, but pointer. We elide the MEM_REF only if the TBAA type exactly matches the value-type. Since TBAA-wise an array is the same as the element type (unless the language says otherwise - see get_alias_set) we could in some cases with arrays elide it. It didn't seem worth the extra complexity in the code doing this, see maybe_canonicalize_mem_ref_addr. Honza is at the moment trying to improve access-path disambiguation which runs into related issues. Richard. > Jakub
> > > Aah, it's two REFs in one. I misread the array index as being > > > a part of the MEM_REF operand, like this: > > > > > > MEM <int[5]> [((int *)&i)[j_1(D)]] = 1; > > > > > > I guess I've never noticed this before. Why is the whole thing > > > not simplified to an ARRAY_REF? > > > > > > i[j_2(D)] = 1; > > > > No idea in this case, though of course there can be other cases e.g. > > where the MEM_REF has different number of elements, different element type > > etc. from the underlying variable or where the MEM_REF first operand is not > > address, but pointer. > > We elide the MEM_REF only if the TBAA type exactly matches the value-type. > Since TBAA-wise an array is the same as the element type (unless the > language says otherwise - see get_alias_set) we could in some cases with > arrays elide it. It didn't seem worth the extra complexity in the code doing > this, see maybe_canonicalize_mem_ref_addr. Honza is at the moment > trying to improve access-path disambiguation which runs into related issues. I have just sent an email with some analysis running into it on vect testcase. I think we ought to solve this - it seems to be relatively common case and my proposed patch makes it worse (and I think for valid reasons) Honza > > Richard. > > > Jakub
diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr19807.C b/gcc/testsuite/g++.dg/tree-ssa/pr19807.C index cbe06b4ce62..08e0dd13115 100644 --- a/gcc/testsuite/g++.dg/tree-ssa/pr19807.C +++ b/gcc/testsuite/g++.dg/tree-ssa/pr19807.C @@ -11,7 +11,8 @@ void foo(void) z = 1 + &a[1]; } -/* { dg-final { scan-tree-dump-times "&MEM\\\[\\\(void .\\\)&a \\\+ 8B\\\]" 3 "optimized" } } */ +/* { dg-final { scan-tree-dump-times "&MEM\\\[\\\(void .\\\)&a \\\+ 8B\\\]" 3 "optimized" { target { ! store_merge } } } } + { dg-final { scan-tree-dump-times "&MEM\\\[\\(int \\*\\)\\\(void .\\\)&a \\\+ 8B\\\]" 3 "optimized" { target { store_merge } } } } */ void bar(int i) diff --git a/gcc/testsuite/g++.dg/tree-ssa/ssa-dse-1.C b/gcc/testsuite/g++.dg/tree-ssa/ssa-dse-1.C index 1fd8dec99e9..da9fc3b9c6a 100644 --- a/gcc/testsuite/g++.dg/tree-ssa/ssa-dse-1.C +++ b/gcc/testsuite/g++.dg/tree-ssa/ssa-dse-1.C @@ -97,5 +97,5 @@ int main() } -/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" } } */ - +/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { ! store_merge } } } } + { dg-final { scan-tree-dump-times "MEM\\\[\\(char\\\[176] \\*\\)\\(struct FixBuf \\*\\)&<retval> \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { store_merge } } } } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/dump-6.c b/gcc/testsuite/gcc.dg/tree-ssa/dump-6.c new file mode 100644 index 00000000000..8b4a51c6cbf --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/dump-6.c @@ -0,0 +1,37 @@ +/* PR middle-end/90676 - default GIMPLE dumps lack information + { dg-do compile } + { dg-options "-O2 -fdump-tree-store-merging" } + { dg-require-effective-target int32plus } + { dg-require-effective-target store_merge } */ + + +extern char a2[2]; + +void f2 (void) +{ + a2[0] = 1; + a2[1] = 0; +} + +extern char a4[4]; + +void f4 (void) +{ + a4[0] = 1; + a4[1] = 0; + a4[2] = 0; + a4[3] = 0; +} + +extern char a8[8]; + +void f8 (void) +{ + a8[0] = 1; + for (int i = 1; i != 8; ++i) + a8[i] = 0; +} + +/* { dg-final { scan-tree-dump "MEM\\\[\\(unsigned short \\*\\)\\(char \\*\\)\\&a2] = " "store-merging"} } + { dg-final { scan-tree-dump "MEM\\\[\\(unsigned int \\*\\)\\(char \\*\\)\\&a4] = " "store-merging"} } + { dg-final { scan-tree-dump "MEM\\\[\\(unsigned long \\*\\)\\(char \\*\\)\\&a8] = " "store-merging"} } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr30375.c b/gcc/testsuite/gcc.dg/tree-ssa/pr30375.c index 4494a2b0bd6..1d47e4c39eb 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/pr30375.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr30375.c @@ -22,5 +22,6 @@ void test_signed_msg_encoding(void) f(); } -/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct _s \\*\\)&signInfo \\+ \[0-9\]+B\\\] = {}" 1 "dse1" } } */ +/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct _s \\*\\)&signInfo \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { ! store_merge } } } } + { dg-final { scan-tree-dump-times "MEM\\\[\\(char\\\[8] \\*\\)\\(struct _s \\*\\)&signInfo \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { store_merge } } } } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/slsr-27.c b/gcc/testsuite/gcc.dg/tree-ssa/slsr-27.c index 35b3d00ee44..ea9aad9c4cc 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/slsr-27.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/slsr-27.c @@ -19,4 +19,5 @@ f (struct x *p, unsigned int n) /* { dg-final { scan-tree-dump-times "\\* 4;" 1 "dom3" { target { int32 } } } } */ /* { dg-final { scan-tree-dump-times "\\* 2;" 1 "dom3" { target { int16 } } } } */ /* { dg-final { scan-tree-dump-times "p_\\d\+\\(D\\) \\+ \[^\r\n\]*_\\d\+;" 1 "dom3" } } */ -/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct x \\*\\)\[^\r\n\]*_\\d\+" 3 "dom3" } } */ +/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct x \\*\\)\[^\r\n\]*_\\d\+" 3 "dom3" { target { ! store_merge } } } } */ +/* { dg-final { scan-tree-dump-times "MEM\\\[\\(int \\*\\)\\(struct x \\*\\)\[^\r\n\]*_\\d\+" 3 "dom3" { target { store_merge } } } } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/slsr-28.c b/gcc/testsuite/gcc.dg/tree-ssa/slsr-28.c index 732d2324db5..37526a3d9fb 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/slsr-28.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/slsr-28.c @@ -23,4 +23,5 @@ f (struct x *p, unsigned int n) /* { dg-final { scan-tree-dump-times "\\* 4;" 1 "dom3" { target { int32 } } } } */ /* { dg-final { scan-tree-dump-times "\\* 2;" 1 "dom3" { target { int16 } } } } */ /* { dg-final { scan-tree-dump-times "p_\\d\+\\(D\\) \\+ \[^\r\n\]*_\\d\+" 1 "dom3" } } */ -/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct x \\*\\)\[^\r\n\]*_\\d\+" 9 "dom3" } } */ +/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct x \\*\\)\[^\r\n\]*_\\d\+" 9 "dom3" { target { ! store_merge } } } } */ +/* { dg-final { scan-tree-dump-times "MEM\\\[\\(int \\*\\)\\(struct x \\*\\)\[^\r\n\]*_\\d\+" 9 "dom3" { target { store_merge } } } } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/slsr-29.c b/gcc/testsuite/gcc.dg/tree-ssa/slsr-29.c index a22cc7906da..8499f2bd28c 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/slsr-29.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/slsr-29.c @@ -25,4 +25,5 @@ f (struct x *p, unsigned int n) /* { dg-final { scan-tree-dump-times "\\* 4;" 1 "dom3" { target { int32 } } } } */ /* { dg-final { scan-tree-dump-times "\\* 2;" 1 "dom3" { target { int16 } } } } */ /* { dg-final { scan-tree-dump-times "p_\\d\+\\(D\\) \\+ \[^\r\n\]*_\\d\+" 1 "dom3" } } */ -/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct x \\*\\)\[^\r\n\]*_\\d\+" 9 "dom3" } } */ +/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct x \\*\\)\[^\r\n\]*_\\d\+" 9 "dom3" { target { ! store_merge } } } } */ +/* { dg-final { scan-tree-dump-times "MEM\\\[\\(int \\*\\)\\(struct x \\*\\)\[^\r\n\]*_\\d\+" 9 "dom3" { target { store_merge } } } } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c index 282194c1e32..709adce32bf 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-24.c @@ -59,4 +59,5 @@ void foo(int prec, bar (&info); } -/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct printf_info \\*\\)&info \\+ \[0-9\]+B\\\] = {}" 1 "dse1" } } */ +/* { dg-final { scan-tree-dump-times "MEM\\\[\\(struct printf_info \\*\\)&info \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { ! store_merge } } } } + { dg-final { scan-tree-dump-times "MEM\\\[\\(char\\\[4] \\*\\)\\(struct printf_info \\*\\)&info \\+ \[0-9\]+B\\\] = {}" 1 "dse1" { target { store_merge } } } } */ diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c index 4ba9170ddd3..649c81e06cd 100644 --- a/gcc/tree-pretty-print.c +++ b/gcc/tree-pretty-print.c @@ -1690,21 +1690,32 @@ dump_generic_node (pretty_printer *pp, tree node, int spc, dump_flags_t flags, } else { - tree ptype; - pp_string (pp, "MEM["); + tree nodetype = TREE_TYPE (node); + tree op0 = TREE_OPERAND (node, 0); + tree op1 = TREE_OPERAND (node, 1); + tree op1type = TYPE_MAIN_VARIANT (TREE_TYPE (op1)); + + if (!tree_int_cst_equal (TYPE_SIZE (nodetype), + TYPE_SIZE (TREE_TYPE (op1type)))) + { + /* If the size of the type of the operand is not the same + as the size of the MEM_REF expression include a cast + to a pointer to the type of the latter to make it clear + how many bytes of memory are being accessed. */ + pp_left_paren (pp); + dump_generic_node (pp, nodetype, spc, flags | TDF_SLIM, false); + pp_string (pp, " *)"); + } + pp_left_paren (pp); - ptype = TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (node, 1))); - dump_generic_node (pp, ptype, - spc, flags | TDF_SLIM, false); + dump_generic_node (pp, op1type, spc, flags | TDF_SLIM, false); pp_right_paren (pp); - dump_generic_node (pp, TREE_OPERAND (node, 0), - spc, flags, false); - if (!integer_zerop (TREE_OPERAND (node, 1))) + dump_generic_node (pp, op0, spc, flags, false); + if (!integer_zerop (op1)) { pp_string (pp, " + "); - dump_generic_node (pp, TREE_OPERAND (node, 1), - spc, flags, false); + dump_generic_node (pp, op1, spc, flags, false); } if ((flags & TDF_ALIAS) && MR_DEPENDENCE_CLIQUE (node) != 0)