On Wed, Jun 13, 2012 at 4:48 AM, Richard Guenther
<richard.guent...@gmail.com> wrote:
> On Fri, Jun 8, 2012 at 7:16 AM, Sharad Singhai <sing...@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 <davi...@google.com> 
>> wrote:
>>> On Wed, Jun 6, 2012 at 10:58 PM, Sharad Singhai <sing...@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.guent...@gmail.com> wrote:
>>>>> On Sat, May 12, 2012 at 6:39 PM, Xinliang David Li <davi...@google.com> 
>>>>> wrote:
>>>>>> On Sat, May 12, 2012 at 9:26 AM, Gabriel Dos Reis
>>>>>> <g...@integrable-solutions.net> wrote:
>>>>>>> On Sat, May 12, 2012 at 11:05 AM, Xinliang David Li 
>>>>>>> <davi...@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

Reply via email to