On Thu, Sep 27, 2012 at 4:35 AM, Sharad Singhai <sing...@google.com> wrote:
> Thanks for the review. A couple of comments inline:
>
>> Some minor issues:
>>
>> * c/c-decl.c (c_write_global_declarations): Use different method to
>> determine if the dump has ben initialized.
>> * cp/decl2.c (cp_write_global_declarations): Ditto.
>> * testsuite/gcc.target/i386/vect-double-1.c: Fix test.
>>
>> these subdirs all have their separate ChangeLog entry from where the
>> directory name is omitted.
>>
>> Index: tree-dump.c
>> ===================================================================
>> --- tree-dump.c (revision 191490)
>> +++ tree-dump.c (working copy)
>> @@ -24,9 +24,11 @@ along with GCC; see the file COPYING3. If not see
>> #include "coretypes.h"
>> #include "tm.h"
>> #include "tree.h"
>> +#include "gimple-pretty-print.h"
>> #include "splay-tree.h"
>> #include "filenames.h"
>> #include "diagnostic-core.h"
>> +#include "rtl.h"
>>
>> what do you need gimple-pretty-print.h and rtl.h for?
>>
>> +
>> +extern void dump_bb (FILE *, basic_block, int, int);
>> +
>>
>> that should be declared in some header
>>
>> +/* Dump gimple statement GS with SPC indentation spaces and
>> + EXTRA_DUMP_FLAGS on the dump streams if DUMP_KIND is enabled. */
>> +
>> +void
>> +dump_gimple_stmt (int dump_kind, int extra_dump_flags, gimple gs, int spc)
>> +{
>>
>> the gimple stuff really belongs in to gimple-pretty-print.c
>
> This dump_gimple_stmt () is just a dispatcher, which uses internal
> data structure such as dump streams/flags. If I move it into
> gimple-pretty-print.c, then I would have to export those
> streams/flags. I was hoping to avoid it by keeping all dump_* ()
> methods together in dumpfile.c (earlier in tree-dump.c). Thus, later
> one could just make dump_file/dump_flags static when all the passes
> have converted to this scheme.
>
You can make the flags/streams global but only expose them via inline
accessors in the header file.
David
>>
>> (parts of tree-dump.c should be moved to a new file dumpfile.c)
>>
>> +/* Dump tree T using EXTRA_DUMP_FLAGS on dump streams if DUMP_KIND is
>> + enabled. */
>> +
>> +void
>> +dump_generic_expr (int dump_kind, int extra_dump_flags, tree t)
>> +{
>>
>> belongs to tree-pretty-print.c (to where the routines are it calls)
>
> This is again a dispatcher for dump_generic_expr () which writes to
> the appropriate stream depending upon dump_kind.
>
>>
>> +int
>> +dump_start (int phase, int *flag_ptr)
>> +{
>>
>> perfect candidate for dumpfile.c
>>
>> You can do this re-shuffling as followup, but please try to not include rtl.h
>> or gimple-pretty-print.h from tree-dump.c. Thus re-shuffling required by
>> that
>> do now. tree-dump.c should only know about dumping 'tree'.
>
> Okay, I have moved relevant methods into dumpfile.c.
>
>>
>> Index: tree-dump.h
>> ===================================================================
>> --- tree-dump.h (revision 191490)
>> +++ tree-dump.h (working copy)
>> @@ -22,6 +22,7 @@ along with GCC; see the file COPYING3. If not see
>> #ifndef GCC_TREE_DUMP_H
>> #define GCC_TREE_DUMP_H
>>
>> +#include "input.h"
>>
>> probably no longer required.
>>
>> Index: dumpfile.h
>> ===================================================================
>> --- dumpfile.h (revision 191490)
>> +++ dumpfile.h (working copy)
>> @@ -22,6 +22,9 @@ along with GCC; see the file COPYING3. If not see
>> #ifndef GCC_DUMPFILE_H
>> #define GCC_DUMPFILE_H 1
>>
>> +#include "coretypes.h"
>> +#include "input.h"
>>
>> likewise for input.h.
>>
>> Index: testsuite/gcc.target/i386/vect-double-1.c
>> ===================================================================
>> --- testsuite/gcc.target/i386/vect-double-1.c (revision 191490)
>> +++ testsuite/gcc.target/i386/vect-double-1.c (working copy)
>> @@ -32,5 +32,5 @@ sse2_test (void)
>> }
>> }
>>
>> -/* { dg-final { scan-tree-dump-times "vectorized 1 loops" 1 "vect" } } */
>> +/* { dg-final { scan-tree-dump-times "Vectorized loops: 1" 1 "vect" } } */
>> /* { dg-final { cleanup-tree-dump "vect" } } */
>>
>> I am sure you need a gazillion more testsuite adjustments? Thus, did you
>> really test the patch by a bootstrap and a toplevel make -k check for
>> regressions?
>>
>> Index: opts.c
>> ===================================================================
>> --- opts.c (revision 191490)
>> +++ opts.c (working copy)
>> @@ -25,6 +25,7 @@ along with GCC; see the file COPYING3. If not see
>> #include "system.h"
>> #include "intl.h"
>> #include "coretypes.h"
>> +#include "dumpfile.h"
>>
>> I don't see that you add a use for this. Please double-check all your
>> include
>> file changes.
>>
>> Index: gimple-pretty-print.c
>> ===================================================================
>> --- gimple-pretty-print.c (revision 191490)
>> +++ gimple-pretty-print.c (working copy)
>> @@ -69,7 +69,7 @@ maybe_init_pretty_print (FILE *file)
>> }
>> ...
>> Index: gimple-pretty-print.h
>> ===================================================================
>> --- gimple-pretty-print.h (revision 191490)
>> +++ gimple-pretty-print.h (working copy)
>> @@ -31,6 +31,6 @@ extern void debug_gimple_seq (gimple_seq);
>> extern void print_gimple_seq (FILE *, gimple_seq, int, int);
>> extern void print_gimple_stmt (FILE *, gimple, int, int);
>> extern void print_gimple_expr (FILE *, gimple, int, int);
>> -extern void dump_gimple_stmt (pretty_printer *, gimple, int, int);
>> +extern void pp_gimple_stmt_1 (pretty_printer *, gimple, int, int);
>>
>> it looks like changes to these files are only renaming of existing
>> dump_ functions
>> to print_ functions. Consider testing and applying those separately (hereby
>> pre-approved).
>>
>> Index: profile.c
>> ===================================================================
>> --- profile.c (revision 191490)
>> +++ profile.c (working copy)
>> @@ -52,6 +52,7 @@ along with GCC; see the file COPYING3. If not see
>>
>> please leave further changes to passes as followup, thus omit changes to this
>> file for the initial commit and submit it separately.
>
> Okay.
>
>>
>>
>> Index: rtl.h
>> ===================================================================
>> --- rtl.h (revision 191490)
>> +++ rtl.h (working copy)
>> @@ -2482,8 +2482,8 @@ extern bool validate_subreg (enum machine_mode, en
>> /* In combine.c */
>> extern unsigned int extended_count (const_rtx, enum machine_mode, int);
>> extern rtx remove_death (unsigned int, rtx);
>> -extern void dump_combine_stats (FILE *);
>> -extern void dump_combine_total_stats (FILE *);
>> +extern void debug_combine_stats (FILE *);
>> +extern void print_combine_total_stats (FILE *);
>> extern rtx make_compound_operation (rtx, enum rtx_code);
>> Index: combine.c
>> ===================================================================
>> --- combine.c (revision 191490)
>> +++ combine.c (working copy)
>> ...
>>
>> Likewise a patch just doing this re-name is pre-approved and should be
>> checked
>> in separately.
>>
>> @@ -410,6 +419,10 @@ handle_common_deferred_options (void)
>> stack_limit_rtx = gen_rtx_SYMBOL_REF (Pmode, ggc_strdup
>> (opt->arg));
>> break;
>>
>> + case OPT_ftree_vectorizer_verbose_:
>> + dump_remap_tree_vectorizer_verbose (opt->arg);
>> + break;
>> +
>>
>> can you please move that function here (opts-global.c) and make it static?
>
> Done.
>
>>
>> Index: Makefile.in
>> ===================================================================
>> --- Makefile.in (revision 191490)
>> +++ Makefile.in (working copy)
>>
>> remember to adjust for any changes you do above
>>
>> Otherwise the patch looks ok to me.
>>
>> Thanks,
>> Richard.
>
> Thanks,
> Sharad
