diff mbox

Don't include DECL_UIDs in tree-sra fancy names for -fdump-final-insns= (PR c++/70594)

Message ID 20160412145427.GV19207@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek April 12, 2016, 2:54 p.m. UTC 
Hi!

Even without the C++ FE changes, I believe there are occassional DECL_UID
differences (I believe the code just cares that the ordering of the uids
is stable), and the fancy DEC_IGNORED names can leak into the
-fdump-final-insns= dumps (e.g. in MEM_EXPRs or REG_EXPRs.

The following patch treats those similarly to how the various TDF_NOUID
flags handle it in the dumps.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-04-12  Jakub Jelinek  <jakub@redhat.com>

	PR c++/70594
	* tree-sra.c (make_fancy_decl_name): Don't add DECL_UID
	into the fancy names if -fdump-final-insns=.


	Jakub

Comments

Richard Biener April 12, 2016, 3:49 p.m. UTC | #1 
On April 12, 2016 4:54:27 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>Even without the C++ FE changes, I believe there are occassional
>DECL_UID
>differences (I believe the code just cares that the ordering of the
>uids
>is stable), and the fancy DEC_IGNORED names can leak into the
>-fdump-final-insns= dumps (e.g. in MEM_EXPRs or REG_EXPRs.
>
>The following patch treats those similarly to how the various TDF_NOUID
>flags handle it in the dumps.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
>2016-04-12  Jakub Jelinek  <jakub@redhat.com>
>
>	PR c++/70594
>	* tree-sra.c (make_fancy_decl_name): Don't add DECL_UID
>	into the fancy names if -fdump-final-insns=.
>
>--- gcc/tree-sra.c.jj	2016-04-12 11:08:10.000000000 +0200
>+++ gcc/tree-sra.c	2016-04-12 11:15:35.519676357 +0200
>@@ -1543,6 +1543,9 @@ make_fancy_decl_name (tree decl)
>   if (name)
>     obstack_grow (&name_obstack, IDENTIFIER_POINTER (name),
> 		  IDENTIFIER_LENGTH (name));
>+  /* Avoid -fcompare-debug issues on DECL_UID differences.  */
>+  else if (flag_dump_final_insns != NULL)
>+    obstack_grow (&name_obstack, "Dxxxx", 5);
>   else
>     {
>       sprintf (buffer, "D%u", DECL_UID (decl));

I'd rather use a separate counter that SRA increments.  We an always dump counter to uid mapping.

Richard.

>
>	Jakub
Jakub Jelinek April 12, 2016, 3:55 p.m. UTC | #2 
On Tue, Apr 12, 2016 at 05:49:26PM +0200, Richard Biener wrote:
> On April 12, 2016 4:54:27 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
> >Hi!
> >
> >Even without the C++ FE changes, I believe there are occassional
> >DECL_UID
> >differences (I believe the code just cares that the ordering of the
> >uids
> >is stable), and the fancy DEC_IGNORED names can leak into the
> >-fdump-final-insns= dumps (e.g. in MEM_EXPRs or REG_EXPRs.
> >
> >The following patch treats those similarly to how the various TDF_NOUID
> >flags handle it in the dumps.
> >
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> >2016-04-12  Jakub Jelinek  <jakub@redhat.com>
> >
> >	PR c++/70594
> >	* tree-sra.c (make_fancy_decl_name): Don't add DECL_UID
> >	into the fancy names if -fdump-final-insns=.
> >
> >--- gcc/tree-sra.c.jj	2016-04-12 11:08:10.000000000 +0200
> >+++ gcc/tree-sra.c	2016-04-12 11:15:35.519676357 +0200
> >@@ -1543,6 +1543,9 @@ make_fancy_decl_name (tree decl)
> >   if (name)
> >     obstack_grow (&name_obstack, IDENTIFIER_POINTER (name),
> > 		  IDENTIFIER_LENGTH (name));
> >+  /* Avoid -fcompare-debug issues on DECL_UID differences.  */
> >+  else if (flag_dump_final_insns != NULL)
> >+    obstack_grow (&name_obstack, "Dxxxx", 5);
> >   else
> >     {
> >       sprintf (buffer, "D%u", DECL_UID (decl));
> 
> I'd rather use a separate counter that SRA increments.  We an always dump counter to uid mapping.

So you mean add another hash table that maps DECL_UIDs to these SRA
counters?  Because if we dump there any number of say FIELD_DECL, it would
be desirable to use the same number on any further fancy names with that
FIELD_DECL.
Do it unconditionally, or just for flag_dump_final_insns?

	Jakub
Richard Biener April 12, 2016, 6:19 p.m. UTC | #3 
On April 12, 2016 5:55:16 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Tue, Apr 12, 2016 at 05:49:26PM +0200, Richard Biener wrote:
>> On April 12, 2016 4:54:27 PM GMT+02:00, Jakub Jelinek
><jakub@redhat.com> wrote:
>> >Hi!
>> >
>> >Even without the C++ FE changes, I believe there are occassional
>> >DECL_UID
>> >differences (I believe the code just cares that the ordering of the
>> >uids
>> >is stable), and the fancy DEC_IGNORED names can leak into the
>> >-fdump-final-insns= dumps (e.g. in MEM_EXPRs or REG_EXPRs.
>> >
>> >The following patch treats those similarly to how the various
>TDF_NOUID
>> >flags handle it in the dumps.
>> >
>> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>> >
>> >2016-04-12  Jakub Jelinek  <jakub@redhat.com>
>> >
>> >	PR c++/70594
>> >	* tree-sra.c (make_fancy_decl_name): Don't add DECL_UID
>> >	into the fancy names if -fdump-final-insns=.
>> >
>> >--- gcc/tree-sra.c.jj	2016-04-12 11:08:10.000000000 +0200
>> >+++ gcc/tree-sra.c	2016-04-12 11:15:35.519676357 +0200
>> >@@ -1543,6 +1543,9 @@ make_fancy_decl_name (tree decl)
>> >   if (name)
>> >     obstack_grow (&name_obstack, IDENTIFIER_POINTER (name),
>> > 		  IDENTIFIER_LENGTH (name));
>> >+  /* Avoid -fcompare-debug issues on DECL_UID differences.  */
>> >+  else if (flag_dump_final_insns != NULL)
>> >+    obstack_grow (&name_obstack, "Dxxxx", 5);
>> >   else
>> >     {
>> >       sprintf (buffer, "D%u", DECL_UID (decl));
>> 
>> I'd rather use a separate counter that SRA increments.  We an always
>dump counter to uid mapping.
>
>So you mean add another hash table that maps DECL_UIDs to these SRA
>counters?  Because if we dump there any number of say FIELD_DECL, it
>would
>be desirable to use the same number on any further fancy names with
>that
>FIELD_DECL.
>Do it unconditionally, or just for flag_dump_final_insns?

I'd have just added sth to the dumps so you can manually connect the XXX in the name with the UID.

Richard.

>	Jakub
Jakub Jelinek April 12, 2016, 6:47 p.m. UTC | #4 
On Tue, Apr 12, 2016 at 08:19:57PM +0200, Richard Biener wrote:
> >So you mean add another hash table that maps DECL_UIDs to these SRA
> >counters?  Because if we dump there any number of say FIELD_DECL, it
> >would
> >be desirable to use the same number on any further fancy names with
> >that
> >FIELD_DECL.
> >Do it unconditionally, or just for flag_dump_final_insns?
> 
> I'd have just added sth to the dumps so you can manually connect the XXX in the name with the UID.

But the names appear in all the gimple, ipa? and rtl dumps after that, so
if it e.g. just printed a SRA uid -> DECL uid mapping in tree-sra*-details
dump, then one would not have it easily accessible when looking all the
other dumps.
If it is just guarded with flag_dump_final_insns != NULL, then we have the
previous behavior almost all the time, just for -fcompare-debug effectively
force -nouid flag into all the dumps for the fancy names.  But it is easy
to just remove that flag from the command line to have more details, IMHO
that is better than just looking up some mapping dump somewhere.

	Jakub
Richard Biener April 13, 2016, 7:25 a.m. UTC | #5 
On Tue, 12 Apr 2016, Jakub Jelinek wrote:

> On Tue, Apr 12, 2016 at 08:19:57PM +0200, Richard Biener wrote:
> > >So you mean add another hash table that maps DECL_UIDs to these SRA
> > >counters?  Because if we dump there any number of say FIELD_DECL, it
> > >would
> > >be desirable to use the same number on any further fancy names with
> > >that
> > >FIELD_DECL.
> > >Do it unconditionally, or just for flag_dump_final_insns?
> > 
> > I'd have just added sth to the dumps so you can manually connect the XXX in the name with the UID.
> 
> But the names appear in all the gimple, ipa? and rtl dumps after that, so
> if it e.g. just printed a SRA uid -> DECL uid mapping in tree-sra*-details
> dump, then one would not have it easily accessible when looking all the
> other dumps.

Sure - but does it really matter?  Usually decls also have names.

> If it is just guarded with flag_dump_final_insns != NULL, then we have the
> previous behavior almost all the time, just for -fcompare-debug effectively
> force -nouid flag into all the dumps for the fancy names.  But it is easy
> to just remove that flag from the command line to have more details, IMHO
> that is better than just looking up some mapping dump somewhere.

I hate to change behavior of passes based on unrelated flags.  
Over-engineering things just in case you need to debug sth is also bad.

So if you think it's not acceptable to drop the relation between
the artificial number used by SRA and the original UID then go
with a hash-map unconditionally.  You still have to dump and lookup
the actual relation though - there's no way around this unless you
hack dump-final-insns to filter DECL names (maybe just make it
strip all DECL_ARTIFICIAL names completely?)

Richard.
diff mbox

Patch

--- gcc/tree-sra.c.jj	2016-04-12 11:08:10.000000000 +0200
+++ gcc/tree-sra.c	2016-04-12 11:15:35.519676357 +0200
@@ -1543,6 +1543,9 @@  make_fancy_decl_name (tree decl)
   if (name)
     obstack_grow (&name_obstack, IDENTIFIER_POINTER (name),
 		  IDENTIFIER_LENGTH (name));
+  /* Avoid -fcompare-debug issues on DECL_UID differences.  */
+  else if (flag_dump_final_insns != NULL)
+    obstack_grow (&name_obstack, "Dxxxx", 5);
   else
     {
       sprintf (buffer, "D%u", DECL_UID (decl));