From patchwork Wed Jun 13 11:48:45 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 164641 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 5A760B6F77 for ; Wed, 13 Jun 2012 21:49:54 +1000 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1340192994; h=Comment: DomainKey-Signature:Received:Received:Received:Received: MIME-Version:Received:Received:In-Reply-To:References:Date: Message-ID:Subject:From:To:Cc:Content-Type: Content-Transfer-Encoding:Mailing-List:Precedence:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=An8Rgzj79XiH8Q9Xt64rrhADxoo=; b=YEWIKwYrnFgKUTc GIC74Ilc/4iuAgjT+FMJWITM9eAB0VmP2XqzvehVsAUImVmtfVnd/mYW+I1BHmib dP/5EM5WF2q4mKNMgAbWi4gA2LS5KpbvP0oVTbL2s4l8e7FLLU++09puWQwhhvJx fwQlW+dO9+dpbZnBbsQJ3FSslZbk= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:MIME-Version:Received:Received:In-Reply-To:References:Date:Message-ID:Subject:From:To:Cc:Content-Type:Content-Transfer-Encoding:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=XAF4uK5ZXJQPC/riERfS8ahN0H4QnyI7mU4rmShPXfzTLI4nsbf5tSvY5qVg0n +ih1UlrbpTg2/SCKJoKXQEhYlpwjfFbVyhiIqzCRNR2Da26O2i9pZRoirLxEXJ8y SqhyDrwdjqcIvoOONu/mYh05+kbO1R8bU6vOJPL8NzKBc=; Received: (qmail 3871 invoked by alias); 13 Jun 2012 11:49:17 -0000 Received: (qmail 3742 invoked by uid 22791); 13 Jun 2012 11:49:09 -0000 X-SWARE-Spam-Status: No, hits=-4.8 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, KHOP_RCVD_TRUST, KHOP_THREADED, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE, TW_TM X-Spam-Check-By: sourceware.org Received: from mail-ob0-f175.google.com (HELO mail-ob0-f175.google.com) (209.85.214.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 13 Jun 2012 11:48:46 +0000 Received: by obhx4 with SMTP id x4so813065obh.20 for ; Wed, 13 Jun 2012 04:48:46 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.18.136 with SMTP id w8mr24715421obd.38.1339588125856; Wed, 13 Jun 2012 04:48:45 -0700 (PDT) Received: by 10.76.82.4 with HTTP; Wed, 13 Jun 2012 04:48:45 -0700 (PDT) In-Reply-To: References: <20120509064637.22949A2081@nabu.mtv.corp.google.com> Date: Wed, 13 Jun 2012 13:48:45 +0200 Message-ID: Subject: Re: [PATCH] Add option for dumping to stderr (issue6190057) From: Richard Guenther To: Sharad Singhai Cc: Xinliang David Li , Gabriel Dos Reis , gcc-patches@gcc.gnu.org, Andrew Pinski X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org On Fri, Jun 8, 2012 at 7:16 AM, Sharad Singhai wrote: > Okay, I have updated the attached patch so that the output from > -ftree-vectorizer-verbose is considered diagnostic information and is > always > sent to stderr. Other functionality remains unchanged. Here is some > more context about this patch. > > This patch improves the dump infrastructure and public interfaces so > that the existing private pass-specific dump stream is separated from > the diagnostic dump stream (typically stderr).  The optimization > passes can output information on the two streams independently. > > The newly defined interfaces are: > > Individual passes do not need to access the dump file directly. Thus Instead > of doing > >   if (dump_file && (flags & dump_flags)) >      fprintf (dump_file, ...); > > they can do > >     dump_printf (flags, ...); > > If the current pass has FLAGS enabled then the information gets > printed into the dump file otherwise not. > > Similar to the dump_printf (), another function is defined, called > >        diag_printf (dump_flags, ...) > > This prints information only onto the diagnostic stream, typically > standard error. It is useful for separating pass-specific dump > information from > the diagnostic information. > > Currently, as a proof of concept, I have converted vectorizer passes > to use the new dump format. For this, I have considered > information printed in vect_dump file as diagnostic. Thus 'fprintf' > calls are changed to 'diag_printf'. Some other information printed to > dump_file is sent to the regular dump file via 'dump_printf ()'. It > helps to separate the two streams because they might serve different > purposes and might have different formatting requirements. > > For example, using the trunk compiler, the following invocation > > g++ -S v.cc -ftree-vectorize -fdump-tree-vect -ftree-vectorizer-verbose=2 > > prints tree vectorizer dump into a file named 'v.cc.113t.vect'. > However, the verbose diagnostic output is silently > ignored. This is not desirable as the two types of dump should not interfere. > > After this patch, the vectorizer dump is available in 'v.cc.113t.vect' > as before, but the verbose vectorizer diagnostic is additionally > printed on stderr. Thus both types of dump information are output. > > An additional feature of this patch is that individual passes can > print dump information into command-line named files instead of auto > numbered filename. For example, I'd wish you'd leave out this part for a followup. > > g++ -S -O2 v.cc -ftree-vectorize -fdump-tree-vect=foo.vect >     -ftree-vectorizer-verbose=2 >     -fdump-tree-pre=foo.pre > > This prints the tree vectorizer dump into 'foo.vect', PRE dump into > 'foo.pre', and the vectorizer verbose diagnostic dump onto stderr. > > Please take another look. if (loop_loc != UNKNOWN_LOC) dump_printf (dump_flags, "\nloop at %s:%d: ", LOC_FILE (loop_loc), LOC_LINE (loop_loc)); dump_gimple_stmt (dump_flags | TDF_SLIM, cond_stmt, 0); for example. I notice that you maybe mis-understood the message classification I asked you to add (maybe I confused you by mentioning to eventually re-use the TDF_* flags). I think you basically provided this message classification by adding two classes by providing both dump_gimple_stmt and diag_gimple_stmt. But still in the above you keep a dump_flags test _and_ you pass in (altered) dump_flags to the dump/diag_gimple_stmt routines. Let me quote them: +void +dump_gimple_stmt (int flags, gimple gs, int spc) +{ + if (dump_file) + print_gimple_stmt (dump_file, gs, spc, flags); +} +void +diag_gimple_stmt (int flags, gimple gs, int spc) +{ + if (alt_dump_file) + print_gimple_stmt (alt_dump_file, gs, spc, flags); +} I'd say it should have been a single function: void dump_gimple_stmt (enum msg_classification, int additional_flags, gimple gs, int spc) { if (msg_classification & go-to-dumpfile && dump_file) print_gimple_stmt (dump_file, gs, spc, dump_flags | additional_flags); if (msg_classification & go-to-alt-dump-file && alt_dump_file && (alt_dump_flags & msg_classification)) print_gimple_stmt (alt_dump_file, gs, spc, alt_dump_flags | additional_flags); } where msg_classification would include things to suitably classify messages for -fopt-info, like MSG_MISSED_OPTIMIZATION, MSG_OPTIMIZED, MSG_NOTE. In another place we have @@ -1648,7 +1642,7 @@ vect_can_advance_ivs_p (loop_vec_info loop_vinfo) /* Analyze phi functions of the loop header. */ if (vect_print_dump_info (REPORT_DETAILS)) - fprintf (vect_dump, "vect_can_advance_ivs_p:"); + diag_printf (TDF_TREE, "vect_can_advance_ivs_p:"); so why's that diag_printf and why TDF_TREE? I suppose you made all dumps to vect_dump diag_* and all dumps to dump_file dump_*? I think it should have been dump_printf (REPORT_DETAILS, "vect_can_advance_ivs_p:"); thus dump_printf only gets the msg-classification and the printf args (dump-flags are only meaningful when passed down to pretty-printers). Thus @@ -1658,8 +1652,8 @@ vect_can_advance_ivs_p (loop_vec_info loop_vinfo) phi = gsi_stmt (gsi); if (vect_print_dump_info (REPORT_DETAILS)) { - fprintf (vect_dump, "Analyze phi: "); - print_gimple_stmt (vect_dump, phi, 0, TDF_SLIM); + diag_printf (TDF_TREE, "Analyze phi: "); + diag_gimple_stmt (TDF_SLIM, phi, 0); } should be dump_printf (REPORT_DETAILS, "Analyze phi: "); dump_gimple_stmt (REPORT_DETAILS, TDF_SLIM, phi, 0); and the if (vect_print_dump_info (REPORT_DETAILS)) should be what the dump infrastructure provides and thus hidden. Eventually we should have a dump_kind (msg-classification) so we can replace it with if (dump_kind (REPORT_DETAILS)) { dump_printf (REPORT_DETAILS, "Analyze phi: "); dump_gimple_stmt (REPORT_DETAILS, TDF_SLIM, phi, 0); } to reduce the runtime overhead when not diagnosing/dumping. @@ -2464,8 +2458,8 @@ vect_create_cond_for_alias_checks (loop_vec_info l } if (vect_print_dump_info (REPORT_VECTORIZED_LOCATIONS)) - fprintf (vect_dump, "created %u versioning for alias checks.\n", - VEC_length (ddr_p, may_alias_ddrs)); + diag_printf (TDF_TREE, "created %u versioning for alias checks.\n", + VEC_length (ddr_p, may_alias_ddrs)); } so here we have a different msg-classification, REPORT_VECTORIZED_LOCATIONS. As we eventually want a central -fopt-report=... we do not want to leave this implementation detail to individual passes but gather them in a central place. Thanks, Richard. > Thanks, > Sharad > > > On Thu, Jun 7, 2012 at 12:19 AM, Xinliang David Li wrote: >> On Wed, Jun 6, 2012 at 10:58 PM, Sharad Singhai wrote: >>> Sorry about the delay. I have finally incorporated all the suggestions >>> and reorganized the dump infrastructure a bit. The attached patch >>> updates vectorizer passes so that instead of accessing global >>> dump_file directly, these passes call dump_printf (FLAG, format, ...). >>> The dump_printf can choose between two streams, one regular pass dump >>> file, and another optional command line provided file. Currently, it >>> doesn't discriminate and all the dump information goes to both the >>> streams. >>> >>> Thus, for example, >>> >>> g++ -O2 v.cc -ftree-vectorize -fdump-tree-vect=foo.v -ftree-vectorizer-verbose=3 >> >> But the default form of dump option (-fdump-tree-vect) no longer >> interferes with -ftree-vectorize-verbose=xxx output right? (this is >> one of the main requirements). One dumped to the primary stream (named >> dump file) and the other to the stderr -- the default diagnostic (alt) >> stream. >> >> David >> >>> >>> will output the verbose vectorizer information in both *.vect file and >>> foo.v file. However, as I have converted only vectorizer passes so >>> far, there is additional information in *.vect file which is not >>> present in foo.v file. Once other passes are converted to use this >>> scheme, then these two dump files should have identical output. >>> >>> Also note that in this patch -fdump-xxx=yyy format does not override >>> any auto named dump files as in my earlier patches. Instead the dump >>> information is output to both places when a command line dump file >>> option is provided. >>> >>> To summarize: >>> - instead of using dump_begin () / dump_end (), the passes should use >>> dump_start ()/dump_finish (). These new variants do not return the >>> dump_file. However, they still set the global dump_file/dump_flags for >>> the benefit of other passes during the transition. >>> - instead of directly printing to the dump_file, as in >>> if (dump_file) >>>  fprintf (dump_file, ...); >>> >>> The passes should do >>> >>> dump_printf (dump_flag, ...); >>> This will output to dump file(s) only when dump_flag is enabled for a >>> given pass. >>> >>> I have bootstrapped and tested it on x86_64. Does it look okay? >>> >>> Thanks, >>> Sharad >>> >>> >>> On Mon, May 14, 2012 at 12:26 AM, Richard Guenther >>> wrote: >>>> On Sat, May 12, 2012 at 6:39 PM, Xinliang David Li wrote: >>>>> On Sat, May 12, 2012 at 9:26 AM, Gabriel Dos Reis >>>>> wrote: >>>>>> On Sat, May 12, 2012 at 11:05 AM, Xinliang David Li wrote: >>>>>> >>>>>>> The downside is that the dump file format will look different from the >>>>>>> stderr output which is less than ideal. >>>>>> >>>>>> BTW, why do people want to use stderr for dumping internal IRs, >>>>>> as opposed to stdout or other files?  That does not sound right. >>>>>> >>>>> >>>>> I was talking about the transformation information difference. In >>>>> stderr (where diagnostics go to), we may have >>>>> >>>>> foo.c: in function 'foo': >>>>> foo.c:5:6: note: loop was vectorized >>>>> >>>>> but in dump file the format for the information may be different, >>>>> unless we want to duplicate the machinery in diagnostics. >>>> >>>> So?  What's the problem with that ("different" diagnostics)? >>>> >>>> Richard. >>>> >>>>> David >>>>> >>>>>> -- Gaby --- tree-vect-loop-manip.c (revision 188325) +++ tree-vect-loop-manip.c (working copy) @@ -789,14 +789,11 @@ slpeel_make_loop_iterate_ntimes (struct loop *loop gsi_remove (&loop_cond_gsi, true); loop_loc = find_loop_location (loop); - if (dump_file && (dump_flags & TDF_DETAILS)) - { - if (loop_loc != UNKNOWN_LOC) - fprintf (dump_file, "\nloop at %s:%d: ", + if (loop_loc != UNKNOWN_LOC) + dump_printf (TDF_DETAILS, "\nloop at %s:%d: ", LOC_FILE (loop_loc), LOC_LINE (loop_loc)); - print_gimple_stmt (dump_file, cond_stmt, 0, TDF_SLIM); - } - + if (dump_flags & TDF_DETAILS) + dump_gimple_stmt (TDF_SLIM, cond_stmt, 0); loop->nb_iterations = niters; I'm confused by this. Why is this not simply