From: Richard Guenther <richard.guenther@gmail.com>
To: Sharad Singhai <singhai@google.com>
Cc: Xinliang David Li <davidxl@google.com>,
Gabriel Dos Reis <gdr@integrable-solutions.net>,
gcc-patches@gcc.gnu.org, Andrew Pinski <pinskia@gmail.com>
Subject: Re: [PATCH] Add option for dumping to stderr (issue6190057)
Date: Wed, 13 Jun 2012 11:49:00 -0000 [thread overview]
Message-ID: <CAFiYyc37y7Q_z_7vHdZe98s6kKpgTJi4_ehmk=6gVnirV0TFDA@mail.gmail.com> (raw)
In-Reply-To: <CAKxPW65ar+ZdAwYtG5ECnCNYH+Jw+hiyPbXR2_bQ1OoXsr8_Rg@mail.gmail.com>
On Fri, Jun 8, 2012 at 7:16 AM, Sharad Singhai <singhai@google.com> 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.
--- 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
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 <davidxl@google.com> wrote:
>> On Wed, Jun 6, 2012 at 10:58 PM, Sharad Singhai <singhai@google.com> 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
>>> <richard.guenther@gmail.com> wrote:
>>>> On Sat, May 12, 2012 at 6:39 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> On Sat, May 12, 2012 at 9:26 AM, Gabriel Dos Reis
>>>>> <gdr@integrable-solutions.net> wrote:
>>>>>> On Sat, May 12, 2012 at 11:05 AM, Xinliang David Li <davidxl@google.com> 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
next prev parent reply other threads:[~2012-06-13 11:49 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-05-09 6:46 Sharad Singhai
2012-05-09 6:52 ` Andrew Pinski
2012-05-09 6:55 ` Gabriel Dos Reis
2012-05-09 7:22 ` Gabriel Dos Reis
2012-05-09 23:58 ` Sharad Singhai
2012-05-10 0:31 ` Xinliang David Li
2012-05-10 8:18 ` Richard Guenther
2012-05-10 16:28 ` Xinliang David Li
2012-05-11 8:49 ` Richard Guenther
2012-05-11 16:07 ` Xinliang David Li
2012-05-11 18:12 ` Xinliang David Li
2012-05-12 10:31 ` Richard Guenther
2012-05-12 16:06 ` Xinliang David Li
2012-05-12 16:26 ` Gabriel Dos Reis
2012-05-12 16:39 ` Xinliang David Li
2012-05-14 7:26 ` Richard Guenther
2012-06-07 6:16 ` Sharad Singhai
2012-06-07 8:08 ` Xinliang David Li
2012-06-08 6:59 ` Sharad Singhai
2012-06-13 6:28 ` Sharad Singhai
2012-06-13 11:49 ` Richard Guenther [this message]
2012-06-14 8:17 ` Sharad Singhai
2012-06-14 9:13 ` Richard Guenther
2012-06-15 8:13 ` Sharad Singhai
2012-06-15 8:40 ` Richard Guenther
2012-07-03 21:08 ` Sharad Singhai
2012-07-04 13:33 ` Richard Guenther
2012-08-24 8:07 ` Sharad Singhai
[not found] ` <CAKxPW67Nn9YSwH_xwFXBM7=y=32s_HpQ4fUAE+E8GoRSsVjxbA@mail.gmail.com>
2012-09-10 18:21 ` Sharad Singhai
2012-09-11 20:16 ` Xinliang David Li
2012-09-12 8:15 ` Sharad Singhai
[not found] ` <CAFiYyc3AJ1C92QhZHTk83rZPgLSqwk+=qgYDeY9HG-p4UPRrMA@mail.gmail.com>
2012-09-12 10:40 ` Richard Guenther
2012-09-12 16:47 ` Xinliang David Li
2012-09-13 11:08 ` Richard Guenther
2012-09-13 15:02 ` Sharad Singhai
2012-09-13 16:10 ` Xinliang David Li
2012-09-18 8:49 ` Sharad Singhai
2012-09-18 15:43 ` Xinliang David Li
2012-09-18 16:26 ` Sharad Singhai
2012-09-19 12:02 ` Richard Guenther
2012-09-20 7:15 ` Sharad Singhai
2012-09-24 20:38 ` Sharad Singhai
2012-09-26 14:13 ` Richard Guenther
2012-09-27 12:27 ` Sharad Singhai
2012-09-27 17:06 ` Xinliang David Li
[not found] ` <CAKxPW65ohN989RuyK+XqJDjOJ4gFVF9hB4+qfP0LfU_6q4i8yw@mail.gmail.com>
2012-10-01 6:37 ` Sharad Singhai
2012-10-01 11:51 ` H.J. Lu
2012-10-01 13:50 ` Sharad Singhai
2012-10-01 13:52 ` H.J. Lu
2012-10-01 13:56 ` Sharad Singhai
2012-10-01 14:00 ` Richard Guenther
2012-10-01 14:07 ` Sharad Singhai
2012-10-01 18:03 ` Michael Meissner
2012-10-01 18:32 ` Michael Meissner
2012-10-01 18:39 ` Gabriel Dos Reis
2012-10-02 5:42 ` Sharad Singhai
2012-10-02 9:31 ` Richard Guenther
2012-10-02 9:36 ` Gabriel Dos Reis
2012-10-01 21:37 ` [PATCH] Fix powerpc breakage, was: " Michael Meissner
2012-10-01 21:42 ` Gabriel Dos Reis
2012-10-01 22:45 ` Xinliang David Li
2012-10-01 23:06 ` Sharad Singhai
2012-10-01 23:11 ` Xinliang David Li
2012-10-02 9:25 ` Richard Guenther
[not found] ` <CAAe5K+WvHvnmT8QvKbs9+kxNwO1xdm=RxqqGU3T=z7tcRzQVZg@mail.gmail.com>
2012-09-13 15:10 ` [PATCH] " Sharad Singhai
2012-05-10 8:19 ` Sharad Singhai
-- strict thread matches above, loose matches on Subject: below --
2012-10-10 12:03 Dominique Dhumieres
2012-05-07 21:59 Sharad Singhai
2012-05-07 22:03 ` Gabriel Dos Reis
2012-05-08 7:06 ` Sharad Singhai
2012-05-08 8:53 ` Richard Guenther
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFiYyc37y7Q_z_7vHdZe98s6kKpgTJi4_ehmk=6gVnirV0TFDA@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=davidxl@google.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=gdr@integrable-solutions.net \
--cc=pinskia@gmail.com \
--cc=singhai@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).