Patchwork Add option for dumping to stderr (issue6190057)

login
register
mail settings
Submitter Richard Guenther
Date June 13, 2012, 11:48 a.m.
Message ID <CAFiYyc37y7Q_z_7vHdZe98s6kKpgTJi4_ehmk=6gVnirV0TFDA@mail.gmail.com>
Download mbox | patch
Permalink /patch/164641/
State New
Headers show

Comments

Richard Guenther - June 13, 2012, 11:48 a.m.
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.


  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
Sharad Singhai - June 14, 2012, 6:58 a.m.
Thanks for your comments. Responses inline.

On Wed, Jun 13, 2012 at 4:48 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> 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.

I thought you wanted all parts together. Anyway, I can remove this part.

>
>>
>> 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);
> }

Okay.

> 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

Okay.

>
>   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

I considered that, however, the vect_print_dump_info () uses
'vect_location' in a way that seems pass-specific. I didn't want to
make generic dump infrastructure aware of vectorizer-specific
reporting. Also, while more refactoring is possible, I don't know how
much people rely on the vectorizer pass dump output format. Anyway, I
will try to work around these limitations.

>
>   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.

Okay. If I understand your comment right, you want all the different
REPORT_xxx handled in one place. Currently, vectorizer has about 10 of
such types in flag_types.h. All of these will become different
dump_kind flags under the new scheme and other passes will be free to
add more classification flags in future. Is that correct? If so, a
concern is that many of these flags could be very pass specific. For
example, since REPORT_VECTORIZED_LOCATION is not meaningful to other
passes, does it even deserve to be a msg classification?

Thanks,
Sharad

>
> 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
Richard Guenther - June 14, 2012, 8:25 a.m.
On Thu, Jun 14, 2012 at 8:58 AM, Sharad Singhai <singhai@google.com> wrote:
> Thanks for your comments. Responses inline.
>
> On Wed, Jun 13, 2012 at 4:48 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> 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.
>
> I thought you wanted all parts together. Anyway, I can remove this part.
>
>>
>>>
>>> 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);
>> }
>
> Okay.
>
>> 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
>
> Okay.
>
>>
>>   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
>
> I considered that, however, the vect_print_dump_info () uses
> 'vect_location' in a way that seems pass-specific. I didn't want to
> make generic dump infrastructure aware of vectorizer-specific
> reporting. Also, while more refactoring is possible, I don't know how
> much people rely on the vectorizer pass dump output format. Anyway, I
> will try to work around these limitations.

Ah, that's a good point - the dump_* routines should get a location argument
then (possibly mimcking what we do for tree builders, have the main
function be dump_*_loc (location_t, ...) and have a #define for dump_* which
passes UNKNOWN_LOCATION to dump_*_loc which can then refrain from
outputting any location prefix).

I suppose nobody relies on the vectorizer pass dump output format but
testcases in our testsuite.

>>
>>   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.
>
> Okay. If I understand your comment right, you want all the different
> REPORT_xxx handled in one place. Currently, vectorizer has about 10 of
> such types in flag_types.h. All of these will become different
> dump_kind flags under the new scheme and other passes will be free to
> add more classification flags in future. Is that correct? If so, a
> concern is that many of these flags could be very pass specific. For
> example, since REPORT_VECTORIZED_LOCATION is not meaningful to other
> passes, does it even deserve to be a msg classification?

Well, I suppose the generic name would be REPORT_OPTIMIZED_LOCATIONS
and REPORT_UNOPTIMIZED_LOCATIONS and if you dump information from
the vectorizer these naturally are about vectorized locations.  Thus,
-fopt-report=optimized,all for alll passes information on
optimizations or -fopt-report=optimized,vect for only vectorizer
information?  How do other compilers
allow to specify detail / transformation kind in this context?

Thus yes, try to use "common" report kinds, but I can envision that eventually
a few very specific ones will pop in.

Thanks,
Richard.

> Thanks,
> Sharad
>
>>
>> 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
Sharad Singhai - June 15, 2012, 5:47 a.m.
On Wed, Jun 13, 2012 at 4:48 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> 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);
> }

Yes,  I can make the single function work for both regular (dump_file)
and diagnostic (stderr for now) dump streams. In fact, my original
patch did just that. However, it duplicated output on *both* the
streams. For example,

gcc -O2 -ftree-vectorize -fdump-tree-vect=foo.vect
-ftree-vectorizer-verbose=2 foo.c

Since both streams are enabled in this case, a call to dump_printf ()
would duplicate the information to both files, i.e., the dump and
diagnostic will be intermixed. Is that intended? My first thought was
to keep them separated via dump_*() and diag_* () methods.

Thanks,
Sharad

> 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
Richard Guenther - June 15, 2012, 8:18 a.m.
On Fri, Jun 15, 2012 at 7:47 AM, Sharad Singhai <singhai@google.com> wrote:
> On Wed, Jun 13, 2012 at 4:48 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> 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);
>> }
>
> Yes,  I can make the single function work for both regular (dump_file)
> and diagnostic (stderr for now) dump streams. In fact, my original
> patch did just that. However, it duplicated output on *both* the
> streams. For example,
>
> gcc -O2 -ftree-vectorize -fdump-tree-vect=foo.vect
> -ftree-vectorizer-verbose=2 foo.c
>
> Since both streams are enabled in this case, a call to dump_printf ()
> would duplicate the information to both files, i.e., the dump and
> diagnostic will be intermixed. Is that intended? My first thought was
> to keep them separated via dump_*() and diag_* () methods.

No, I think that's intended.  The regular dump-files should contain
everything, the secondary stream (eventually enabled by -fopt-info)
should be a filtered regular dump-file.

Thanks,
Richard.

> Thanks,
> Sharad
>
>> 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
Sharad Singhai - July 3, 2012, 9:07 p.m.
Apologies for the spam. Attempting to resend the patch after shrinking it.

I have updated the attached patch to use a new dump message
classification system for the vectorizer. It currently uses four
classes, viz, MSG_OPTIMIZED_LOCATIONS, MSG_UNOPTIMIZED_LOCATION,
MSG_MISSING_OPTIMIZATION, and MSG_NOTE. I have gone through the
vectorizer passes and have converted each call to fprintf (dump_file,
....) to a message classification matching in spirit. Most often, it
is MSG_OPTIMIZED_LOCATIONS, but occasionally others as well.

For example, the following

if (vect_print_dump_info (REPORT_DETAILS))
  {
    fprintf (vect_dump, "niters for prolog loop: ");
    print_generic_expr (vect_dump, iters, TDF_SLIM);
  }

gets converted to

if (dump_kind (MSG_OPTIMIZED_LOCATIONS))
  {
     dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, vect_location,
                      "niters for prolog loop: ");
     dump_generic_expr (MSG_OPTIMIZED_LOCATIONS, TDF_SLIM, iters);
  }

The asymmetry between the first printf and the second is due to the
fact that 'vect_print_dump_info (xxx)' prints the location as a
"side-effect". To preserve the original intent somewhat, I have
converted the first call within a dump sequence to a dump_printf_loc
(xxx) which prints the location while the subsequence calls within the
same conditional get converted to the corresponding plain variants.

I considered removing the support for alternate dump file, but ended
up preserving it instead since it is needed for setting the alternate
dump file to stderr for the case when -fopt-info is given but no dump
file is available.

The following invocation
g++ ... -ftree-vectorize -fopt-info=4

dumps *all* available information to stderr. Currently, the opt-info
level is common to all passes, i.e., a pass can't specify if wants a
different level of diagnostic info. This can be added as an
enhancement with a suitable syntax for selecting passes.

I haven't fixed up the documentation/tests but wanted to get some
feedback about the current state of patch before doing that.

Thanks,
Sharad

On Fri, Jun 15, 2012 at 1:18 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Fri, Jun 15, 2012 at 7:47 AM, Sharad Singhai <singhai@google.com> wrote:
>> On Wed, Jun 13, 2012 at 4:48 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> 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);
>>> }
>>
>> Yes,  I can make the single function work for both regular (dump_file)
>> and diagnostic (stderr for now) dump streams. In fact, my original
>> patch did just that. However, it duplicated output on *both* the
>> streams. For example,
>>
>> gcc -O2 -ftree-vectorize -fdump-tree-vect=foo.vect
>> -ftree-vectorizer-verbose=2 foo.c
>>
>> Since both streams are enabled in this case, a call to dump_printf ()
>> would duplicate the information to both files, i.e., the dump and
>> diagnostic will be intermixed. Is that intended? My first thought was
>> to keep them separated via dump_*() and diag_* () methods.
>
> No, I think that's intended.  The regular dump-files should contain
> everything, the secondary stream (eventually enabled by -fopt-info)
> should be a filtered regular dump-file.
>
> Thanks,
> Richard.
>
>> Thanks,
>> Sharad
>>
>>> 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

Patch

--- 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