On Mon, Jun 3, 2019 at 5:13 PM Martin Sebor <mse...@gmail.com> wrote: > > On 6/3/19 4:34 AM, Richard Biener wrote: > > On Mon, Jun 3, 2019 at 10:57 AM Jakub Jelinek <ja...@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 >
