Re: [patch] More cleanups for CFG dumping

[Richard Guenther]

On Wed, Jul 18, 2012 at 7:18 PM, Steven Bosscher <stevenb....@gmail.com> wrote:
> On Wed, Jul 18, 2012 at 10:08 AM, Steven Bosscher <stevenb....@gmail.com> 
> wrote:
>> On Wed, Jul 18, 2012 at 2:24 AM, H.J. Lu <hjl.to...@gmail.com> wrote:
>>> This caused:
>>>
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54008
>>
>> Yes, they failed in my testing, too. I must have been blind to overlook 
>> them...
>>
>> I received some comments in private about the "new look" of the dumps,
>> that I'll be addressing with a patch later today. I'll fix these two
>> test cases with that patch also.
>
> Hello,
>
> The attached patch further consolidates RTL and GIMPLE CFG dumping. It
> also fixes a couple of formatting issues, and it fixes the two broken
> test case patterns from yesterday's patch.
>
> I also ran into a bug in the GIMPLE CFG dumping hooks while working on
> value profiling cleanups (that have been oh so long on my TODO
> list...). It is necessary to flush the pretty-printer buffer to the
> dump file before printing directly to the pretty-printer stream with
> dump_histograms_for_stmt. For good measure, I replaced pp_newfile with
> pp_flush in places where that seemed to be more appropriate, and added
> comments to the functions that do _not_ flush the buffer (this can
> happen e.g. because a user of those functions wants to print
> additional information before the newline that pp_flush adds).

Hmm, pp_flush looks like a lot more expensive compared to pp_newline
for example in

@@ -74,7 +74,7 @@ maybe_init_pretty_print (FILE *file)
 static void
 newline_and_indent (pretty_printer *buffer, int spc)
 {
-  pp_newline (buffer);
+  pp_flush (buffer);
   INDENT (spc);
 }

And I'm pretty sure that newline_and_indent callers that after it directly
dump to the stream should be fixed instead.  In fact, constant flushing
will just make things slow (yes, it's only dumping ...).

Of course

@@ -2256,7 +2261,7 @@ gimple_dump_bb_buff (pretty_printer *buf

       INDENT (curr_indent);
       dump_gimple_stmt (buffer, stmt, curr_indent, flags);
-      pp_newline (buffer);
+      pp_flush (buffer);
       dump_histograms_for_stmt (cfun, buffer->buffer->stream, stmt);
     }

shows one of this "bogus" usages - indeed a pp_flush is required before
dumping directly to the stream.

So, can you please leave out the newline_and_indent change and the following
ones?  The caller that constructed the pretty-printer is responsible for
flushing before destroying it again.

@@ -128,7 +130,7 @@ dump_gimple_seq (pretty_printer *buffer,
       INDENT (spc);
       dump_gimple_stmt (buffer, gs, spc, flags);
       if (!gsi_one_before_end_p (i))
-       pp_newline (buffer);
+       pp_flush (buffer);
     }
 }

@@ -895,6 +897,7 @@ dump_gimple_bind (pretty_printer *buffer
     pp_character (buffer, '>');
   else
     pp_character (buffer, '}');
+  pp_flush (buffer);
 }

@@ -2134,7 +2139,7 @@ dump_phi_nodes (pretty_printer *buffer,
           INDENT (indent);
           pp_string (buffer, "# ");
           dump_gimple_phi (buffer, phi, indent, flags);
-          pp_newline (buffer);
+          pp_flush (buffer);
         }
     }
 }
@@ -2194,7 +2199,7 @@ dump_implicit_edges (pretty_printer *buf
       pp_string (buffer, "else");
       newline_and_indent (buffer, indent + 2);
       pp_cfg_jump (buffer, false_edge->dest);
-      pp_newline (buffer);
+      pp_flush (buffer);
       return;
     }
@@ -2225,7 +2230,7 @@ dump_implicit_edges (pretty_printer *buf
        }

       pp_cfg_jump (buffer, e->dest);
-      pp_newline (buffer);
+      pp_flush (buffer);
     }
 }

Ok with that changes.

Thanks,
Richard.

>
> Bootstrapped&tested on powerpc64-unknown-linux-gnu. This patch doesn't
> affect Graphite, so perhaps it won't break this time :-)
> OK for trunk?
>
> Ciao!
> Steven