From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 83444 invoked by alias); 17 Aug 2018 18:24:24 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 83260 invoked by uid 89); 17 Aug 2018 18:24:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 17 Aug 2018 18:24:10 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 26BA2317C409; Fri, 17 Aug 2018 18:24:09 +0000 (UTC) Received: from ovpn-116-201.phx2.redhat.com (ovpn-116-201.phx2.redhat.com [10.3.116.201]) by smtp.corp.redhat.com (Postfix) with ESMTP id 744CF600C3; Fri, 17 Aug 2018 18:24:08 +0000 (UTC) Message-ID: <1534530247.6963.2.camel@redhat.com> Subject: Re: [PATCH] v2: Formatted printing for dump_* in the middle-end From: David Malcolm To: Jeff Law , Joseph Myers , Richard Biener Cc: GCC Patches Date: Fri, 17 Aug 2018 18:24:00 -0000 In-Reply-To: References: <1533232447-54802-1-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg01052.txt.bz2 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