Resend to gcc-patches
I have addressed the comments by fixing all the minor issues,
bootstrapped and tested on x86_64. I did the recommended reshuffling
by moving non-tree code from tree-dump.c into a new file dumpfile.c.
I committed two successive revisions
r191883 Main patch with the dump infrastructure changes. However, I
accidentally left out a new file, dumpfile.c.
r191884 Added dumpfile.c, and did the renaming of dump_* functions
from gimple_pretty_print.[ch].
As things stand right now, r191883 is broken because of the missing
file 'dumpfile.c', which the very next commit fixes. Anyone who got
broken revision r191883, please svn update. I am really very sorry
about that.
I have a couple more minor patches which deal with renaming; I plan to
address those later.
Thanks,
Sharad
> On Thu, Sep 27, 2012 at 9:10 AM, Xinliang David Li <davi...@google.com>
> wrote:
>>
>> 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
>
>
