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

[Jeff Law <law@redhat.com>]

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.

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.

jeff