Re: [PATCH] v2: Formatted printing for dump_* in the middle-end

[David Malcolm <dmalcolm at redhat dot com>]

On Thu, 2018-08-16 at 22:08 -0600, Jeff Law wrote:
> On 08/02/2018 11:54 AM, David Malcolm wrote:
> > On Tue, 2018-07-31 at 19:56 +0000, Joseph Myers wrote:
> > > On Tue, 31 Jul 2018, David Malcolm wrote:
> > > 
> > > > I didn't exhaustively check every callsite to the changed
> > > > calls;
> > > > I'm
> > > > assuming that -Wformat during bootstrap has effectively checked
> > > > that
> > > > for me.  Though now I think about it, I note that we use
> > > > HOST_WIDE_INT_PRINT_DEC in many places: is this guaranteed to
> > > > be a
> > > > valid input to pp_format on all of our configurations?
> > > 
> > > HOST_WIDE_INT_PRINT_DEC should not be considered safe with
> > > pp_format 
> > > (although since r197049 may have effectively stopped using %I64
> > > on
> > > MinGW 
> > > hosts, I'm not sure if there are current cases where it won't
> > > work).  
> > > Rather, it is the job of pp_format to map the 'w' length
> > > specifier
> > > to 
> > > HOST_WIDE_INT_PRINT_DEC etc.
> > > 
> > > I think it clearly makes for cleaner code to limit use of 
> > > HOST_WIDE_INT_PRINT_* to as few places as possible and to prefer
> > > use
> > > of 
> > > internal printf-like functions that accept formats such as %wd
> > > where 
> > > possible.
> > 
> > Thanks.
> > 
> > I grepped the tree for every use of HOST_WIDE_INT_PRINT* and found
> > all that were within dump_printf[_loc] calls.  All such uses were
> > within files of the form "gcc/tree-vect*.c".
> > 
> > Here's an updated version of the patch (v2) which fixes those
> > callsites to use "%wd" or "%wu"; the dumpfile.c changes are as
> > per v1.
> > 
> > Changed in v2:
> > * rebased to after r263239 (which also touched c-format.c/h)
> > * avoid HOST_WIDE_INT_PRINT* within dump_printf* calls (in
> >   gcc/tree-vect*.c)
> > 
> > I didn't add test coverage for %wd and %wu, as pretty-print.c
> > already
> > has selftest coverage for these.
> > 
> > Richard's review of the v1 patch was:
> > "The patch is OK if C family maintainers agree on their parts."
> > so I'm looking for review of:
> > (a) the c-format.c changes (Joseph?), and
> > (b) the tree-vect*.c changes (Richard?  Joseph?)
> > 
> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> > 
> > Is this patch OK for trunk?
> > 
> > Thanks
> > Dave
> > 
> > 
> > Blurb from v1, for reference:
> > 
> > This patch converts dump_print and dump_printf_loc from using
> > printf (and thus ATTRIBUTE_PRINTF) to using a new pretty-printer
> > based on pp_format, which supports formatting middle-end types.
> > 
> > In particular, the following codes are implemented (in addition
> > to the standard pretty_printer ones):
> > 
> >    %E: gimple *:
> >        Equivalent to: dump_gimple_expr (MSG_*, TDF_SLIM, stmt, 0)
> >    %G: gimple *:
> >        Equivalent to: dump_gimple_stmt (MSG_*, TDF_SLIM, stmt, 0)
> >    %T: tree:
> >        Equivalent to: dump_generic_expr (MSG_*, arg, TDF_SLIM).
> > 
> > Hence it becomes possible to convert e.g.:
> > 
> >   if (dump_enabled_p ())
> >     {
> >       dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> >                        "not vectorized: different sized vector "
> >                        "types in statement, ");
> >       dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> > vectype);
> >       dump_printf (MSG_MISSED_OPTIMIZATION, " and ");
> >       dump_generic_expr (MSG_MISSED_OPTIMIZATION, TDF_SLIM,
> > nunits_vectype);
> >       dump_printf (MSG_MISSED_OPTIMIZATION, "\n");
> >     }
> > 
> > into a one-liner:
> > 
> >   if (dump_enabled_p ())
> >     dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> >                      "not vectorized: different sized vector "
> >                      "types in statement, %T and %T\n",
> >                      vectype, nunits_vectype);
> > 
> > Unlike regular pretty-printers, this one captures optinfo_item
> > instances for the formatted chunks as appropriate, so that when
> > written out to a JSON optimization record, the relevant parts of
> > the message are labelled by type, and by source location (so that
> > e.g. %G is entirely equivalent to using dump_gimple_stmt).
> > 
> > dump_printf and dump_printf_loc become marked with
> > ATTRIBUTE_GCC_DUMP_PRINTF, which the patch also implements.
> > 
> > gcc/c-family/ChangeLog:
> > 	* c-format.c (enum format_type): Add
> > gcc_dump_printf_format_type.
> > 	(gcc_dump_printf_length_specs): New.
> > 	(gcc_dump_printf_flag_pairs): New.
> > 	(gcc_dump_printf_flag_specs): New.
> > 	(gcc_dump_printf_char_table): New.
> > 	(format_types_orig): Add entry for "gcc_dump_printf".
> > 	(init_dynamic_diag_info): Set up length_char_specs and
> > 	conversion_specs for gcc_dump_printf_format_type.
> > 	(handle_format_attribute): Handle gcc_dump_printf_format_type.
> > 
> > gcc/ChangeLog:
> > 	* dump-context.h: Include "dumpfile.h".
> > 	(dump_context::dump_printf_va): Convert final param from
> > va_list
> > 	to va_list *.  Convert from ATTRIBUTE_PRINTF to
> > 	ATTRIBUTE_GCC_DUMP_PRINTF.
> > 	(dump_context::dump_printf_loc_va): Likewise.
> > 	* dumpfile.c: Include "stringpool.h".
> > 	(make_item_for_dump_printf_va): Delete.
> > 	(make_item_for_dump_printf): Delete.
> > 	(class dump_pretty_printer): New class.
> > 	(dump_pretty_printer::dump_pretty_printer): New ctor.
> > 	(dump_pretty_printer::emit_items): New member function.
> > 	(dump_pretty_printer::emit_any_pending_textual_chunks): New
> > member
> > 	function.
> > 	(dump_pretty_printer::emit_item): New member function.
> > 	(dump_pretty_printer::stash_item): New member function.
> > 	(dump_pretty_printer::format_decoder_cb): New member function.
> > 	(dump_pretty_printer::decode_format): New member function.
> > 	(dump_context::dump_printf_va): Reimplement in terms of
> > 	dump_pretty_printer.
> > 	(dump_context::dump_printf_loc_va): Convert final param from
> > va_list
> > 	to va_list *.
> > 	(dump_context::begin_scope): Reimplement call to
> > 	make_item_for_dump_printf.
> > 	(dump_printf): Update for change to dump_printf_va.
> > 	(dump_printf_loc): Likewise.
> > 	(selftest::test_capture_of_dump_calls): Convert "stmt" from
> > 	greturn * to gimple *.  Add a test_decl.  Add tests of
> > dump_printf
> > 	with %T, %E, and %G.
> > 	* dumpfile.h (ATTRIBUTE_GCC_DUMP_PRINTF): New macro.
> > 	(dump_printf): Replace ATTRIBUTE_PRINTF_2 with
> > 	ATTRIBUTE_GCC_DUMP_PRINTF (2, 3).
> > 	(dump_printf_loc): Replace ATTRIBUTE_PRINTF_3 with
> > 	ATTRIBUTE_GCC_DUMP_PRINTF (3, 0).
> > 	* tree-vect-data-refs.c (vect_lanes_optab_supported_p): Convert
> > 	use of HOST_WIDE_INT_PRINT_DEC on unsigned HOST_WIDE_INT
> > "count"
> > 	within a dump_printf_loc call to "%wu".
> > 	(vector_alignment_reachable_p): Merge two dump_printf[_loc]
> > calls,
> > 	converting a use of HOST_WIDE_INT_PRINT_DEC to "%wd".  Add a
> > 	missing space after "=".
> > 	* tree-vect-loop.c (vect_analyze_loop_2) Within a dump_printf
> > 	call, convert use of HOST_WIDE_INT_PRINT_DEC to "%wd".
> > 	* tree-vect-slp.c (vect_slp_bb): Within a dump_printf_loc call,
> > 	convert use of HOST_WIDE_INT_PRINT_UNSIGNED to "%wu".
> > 	* tree-vectorizer.c (try_vectorize_loop_1): Likewise.  Remove
> > 	duplicate "vectorized" from message.
> > 
> > gcc/testsuite/ChangeLog:
> > 	* gcc.dg/format/gcc_diag-1.c: Fix typo.  Add test coverage for
> > 	gcc_dump_printf.
> > 	* gcc.dg/format/gcc_diag-10.c: Add gimple typedef.  Add test
> > 	coverage for gcc_dump_printf.
> > 
> > diff --git a/gcc/dump-context.h b/gcc/dump-context.h
> > index f40ea14..54095d3 100644
> > --- a/gcc/dump-context.h
> > +++ b/gcc/dump-context.h
> > @@ -22,6 +22,7 @@ along with GCC; see the file COPYING3.  If not
> > see
> >  #ifndef GCC_DUMP_CONTEXT_H
> >  #define GCC_DUMP_CONTEXT_H 1
> >  
> > +#include "dumpfile.h" // for ATTRIBUTE_GCC_DUMP_PRINTF
> 
> No real need to document why you include something.  Just include
> what
> you need.  If the need goes away, we do have tools which will help
> identify unnecessary and duplicated #includes.

I've removed the comment.

> Per your comments above, I really only looked at the tree-vect*
> changes,
> which are fine.  So I think you're covered now.  OK for the trunk.

Thanks.

This is now in trunk as r263626 (I rebased and retested it before
committing).

Dave