The dump-pass patch with test case. David
On Tue, Jun 7, 2011 at 11:54 AM, Xinliang David Li <davi...@google.com> wrote: > Please review the attached two patches. > > In the first patch, gate functions are cleaned up. All the per > function legality checks are moved into the executor and the > optimization heuristic checks (optimize for size) remain in the > gators. These allow the the following overriding order: > > common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler > heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass > options <--- legality check > > Testing under going. Ok for trunk? > > Thanks, > > David > > On Tue, Jun 7, 2011 at 9:24 AM, Xinliang David Li <davi...@google.com> wrote: >> Ok -- that sounds good. >> >> David >> >> On Tue, Jun 7, 2011 at 3:10 AM, Richard Guenther >> <richard.guent...@gmail.com> wrote: >>> On Mon, Jun 6, 2011 at 6:00 PM, Xinliang David Li <davi...@google.com> >>> wrote: >>>> On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther >>>> <richard.guent...@gmail.com> wrote: >>>>> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davi...@google.com> >>>>> wrote: >>>>>> This is the version of the patch that walks through pass lists. >>>>>> >>>>>> Ok with this one? >>>>> >>>>> +/* Dump all optimization passes. */ >>>>> + >>>>> +void >>>>> +dump_passes (void) >>>>> +{ >>>>> + struct cgraph_node *n, *node = NULL; >>>>> + tree save_fndecl = current_function_decl; >>>>> + >>>>> + fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid); >>>>> >>>>> this isn't accurate info - cloning can cause more cgraph nodes to >>>>> appear (it also looks completely unrelated to dump_passes ...). >>>>> Please drop it. >>>> >>>> Ok. >>>> >>>> >>>>> >>>>> + create_pass_tab(); >>>>> + gcc_assert (pass_tab); >>>>> >>>>> you have quite many asserts of this kind - we don't want them when >>>>> the previous stmt as in this case indicates everything is ok. >>>> >>>> ok. >>>> >>>>> >>>>> + push_cfun (DECL_STRUCT_FUNCTION (node->decl)); >>>>> >>>>> this has side-effects, I'm not sure we want this here. Why do you >>>>> need it? Probably because of >>>>> >>>>> + is_really_on = override_gate_status (pass, current_function_decl, >>>>> is_on); >>>>> >>>>> ? But that is dependent on the function given which should have no >>>>> effect (unless it is overridden globally in which case >>>>> override_gate_status >>>>> and friends should deal with a NULL cfun). >>>> >>>> As we discussed, currently some pass gate functions depend on per node >>>> information -- those checks need to be pushed into execute functions. >>>> I would like to clean those up later -- at which time, the push/pop >>>> can be removed. >>> >>> I'd like to do it the other way around, first clean up the gate functions >>> then >>> drop in this patch without the cfun push/pop. The revised patch looks ok >>> to me with the cfun push/pop removed. >>> >>> Thanks, >>> Richard. >>> >>>>> >>>>> I don't understand why you need another table mapping pass to name >>>>> when pass->name is available and the info is trivially re-constructible. >>>> >>>> This is needed as the pass->name is not the canonicalized name (i.e., >>>> not with number suffix etc), so the extra mapping from id to >>>> normalized name is needed. >>>> >>>> Thanks, >>>> >>>> David >>>> >>>>> >>>>> Thanks, >>>>> Richard. >>>>> >>>>>> David >>>>>> >>>>>> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davi...@google.com> >>>>>> wrote: >>>>>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther >>>>>>> <richard.guent...@gmail.com> wrote: >>>>>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davi...@google.com> >>>>>>>> wrote: >>>>>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther >>>>>>>>> <richard.guent...@gmail.com> wrote: >>>>>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li >>>>>>>>>> <davi...@google.com> wrote: >>>>>>>>>>> The following patch implements the a new option that dumps gcc PASS >>>>>>>>>>> configuration. The sample output is attached. There is one >>>>>>>>>>> limitation: some placeholder passes that are named with '*xxx' are >>>>>>>>>>> note registered thus they are not listed. They are not important as >>>>>>>>>>> they can not be turned on/off anyway. >>>>>>>>>>> >>>>>>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a >>>>>>>>>>> list >>>>>>>>>>> of function assembler names to be specified. >>>>>>>>>>> >>>>>>>>>>> Ok for trunk? >>>>>>>>>> >>>>>>>>>> Please split the patch. >>>>>>>>>> >>>>>>>>>> I'm not too happy how you dump the pass configuration. Why not >>>>>>>>>> simply, >>>>>>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of >>>>>>>>>> it >>>>>>>>>> at pass execution time when it's not already dumped - that really >>>>>>>>>> looks >>>>>>>>>> gross. >>>>>>>>> >>>>>>>>> Yes, that was the original plan -- but it has problems >>>>>>>>> 1) the dumper needs to know the root pass lists -- which can change >>>>>>>>> frequently -- it can be a long term maintanance burden; >>>>>>>>> 2) the centralized dumper needs to be done after option processing >>>>>>>>> 3) not sure if gate functions have any side effects or have >>>>>>>>> dependencies on cfun >>>>>>>>> >>>>>>>>> The proposed solutions IMHO is not that intrusive -- just three hooks >>>>>>>>> to do the dumping and tracking indentation. >>>>>>>> >>>>>>>> Well, if you have a CU that is empty or optimized to nothing at some >>>>>>>> point >>>>>>>> you will not get a complete pass list. I suppose optimize attributes >>>>>>>> might >>>>>>>> also confuse output. Your solution might not be that intrusive >>>>>>>> but it is still ugly. I don't see 1) as an issue, for 2) you can just >>>>>>>> call the >>>>>>>> dumping from toplev_main before calling do_compile (), 3) gate >>>>>>>> functions >>>>>>>> shouldn't have side-effects, but as they could gate on >>>>>>>> optimize_for_speed () >>>>>>>> your option summary output will be bogus anyway. >>>>>>>> >>>>>>>> So - what is the output intended for if it isn't reliable? >>>>>>> >>>>>>> This needs to be cleaned up at some point -- the gate function should >>>>>>> behave the same for all functions and per-function decisions need to >>>>>>> be pushed down to the executor body. I will try to rework the patch >>>>>>> as you suggested to see if there are problems. >>>>>>> >>>>>>> David >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> Richard. >>>>>>>> >>>>>>>>>> >>>>>>>>>> The documentation should also link this option to the >>>>>>>>>> -fenable/disable >>>>>>>>>> options as obviously the pass names in that dump are those to be >>>>>>>>>> used for those flags (and not readily available anywhere else). >>>>>>>>> >>>>>>>>> Ok. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> I also think that it would be way more useful to note in the >>>>>>>>>> individual >>>>>>>>>> dump files the functions (at the place they would usually appear) >>>>>>>>>> that >>>>>>>>>> have the pass explicitly enabled/disabled. >>>>>>>>> >>>>>>>>> Ok -- for ipa passes or tree/rtl passes where all functions are >>>>>>>>> explicitly disabled. >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> >>>>>>>>> David >>>>>>>>> >>>>>>>>>> >>>>>>>>>> Richard. >>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> >>>>>>>>>>> David >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
Index: doc/invoke.texi =================================================================== --- doc/invoke.texi (revision 174759) +++ doc/invoke.texi (working copy) @@ -291,6 +291,7 @@ Objective-C and Objective-C++ Dialects}. -fdump-translation-unit@r{[}-@var{n}@r{]} @gol -fdump-class-hierarchy@r{[}-@var{n}@r{]} @gol -fdump-ipa-all -fdump-ipa-cgraph -fdump-ipa-inline @gol +-fdump-passes @gol -fdump-statistics @gol -fdump-tree-all @gol -fdump-tree-original@r{[}-@var{n}@r{]} @gol @@ -5069,7 +5070,8 @@ seperated list of function ranges. Each The range is inclusive in both ends. If the range is trivial, the number pair can be simplified a a single number. If the function's cgraph node's @var{uid} is falling within one of the specified ranges, the @var{pass} is disabled for that function. -The @var{uid} is shown in the function header of a dump file. +The @var{uid} is shown in the function header of a dump file, and pass names can be +dumped by using option @option{-fdump-passes}. @item -fdisable-tree-@var{pass} @item -fdisable-tree-@var{pass}=@var{range-list} @@ -5492,6 +5494,11 @@ Dump after function inlining. @end table +@item -fdump-passes +@opindex fdump-passes +Dump the list of optimization passes that are turned on and off by +the current command line options. + @item -fdump-statistics-@var{option} @opindex fdump-statistics Enable and control dumping of pass statistics in a separate file. The Index: tree-pass.h =================================================================== --- tree-pass.h (revision 174759) +++ tree-pass.h (working copy) @@ -639,5 +639,6 @@ extern void do_per_function_toporder (vo extern void disable_pass (const char *); extern void enable_pass (const char *); +extern void dump_passes (void); #endif /* GCC_TREE_PASS_H */ Index: cgraphunit.c =================================================================== --- cgraphunit.c (revision 174759) +++ cgraphunit.c (working copy) @@ -1117,6 +1117,9 @@ cgraph_finalize_compilation_unit (void) fflush (stderr); } + if (flag_dump_passes) + dump_passes (); + /* Gimplify and lower all functions, compute reachability and remove unreachable nodes. */ cgraph_analyze_functions (); Index: common.opt =================================================================== --- common.opt (revision 174759) +++ common.opt (working copy) @@ -1012,6 +1012,10 @@ fdump-noaddr Common Report Var(flag_dump_noaddr) Suppress output of addresses in debugging dumps +fdump-passes +Common Var(flag_dump_passes) Init(0) +Dump optimization passes + fdump-unnumbered Common Report Var(flag_dump_unnumbered) Suppress output of instruction numbers, line number notes and addresses in debugging dumps Index: passes.c =================================================================== --- passes.c (revision 174759) +++ passes.c (working copy) @@ -478,7 +478,7 @@ passr_eq (const void *p1, const void *p2 return !strcmp (s1->unique_name, s2->unique_name); } -static htab_t pass_name_tab = NULL; +static htab_t name_to_pass_map = NULL; /* Register PASS with NAME. */ @@ -488,11 +488,11 @@ register_pass_name (struct opt_pass *pas struct pass_registry **slot; struct pass_registry pr; - if (!pass_name_tab) - pass_name_tab = htab_create (256, passr_hash, passr_eq, NULL); + if (!name_to_pass_map) + name_to_pass_map = htab_create (256, passr_hash, passr_eq, NULL); pr.unique_name = name; - slot = (struct pass_registry **) htab_find_slot (pass_name_tab, &pr, INSERT); + slot = (struct pass_registry **) htab_find_slot (name_to_pass_map, &pr, INSERT); if (!*slot) { struct pass_registry *new_pr; @@ -506,6 +506,101 @@ register_pass_name (struct opt_pass *pas return; /* Ignore plugin passes. */ } +/* Map from pass id to canonicalized pass name. */ + +typedef const char *char_ptr; +DEF_VEC_P(char_ptr); +DEF_VEC_ALLOC_P(char_ptr, heap); +static VEC(char_ptr, heap) *pass_tab = NULL; + +/* Callback function for traversing NAME_TO_PASS_MAP. */ + +static int +pass_traverse (void **slot, void *data ATTRIBUTE_UNUSED) +{ + struct pass_registry **p = (struct pass_registry **)slot; + struct opt_pass *pass = (*p)->pass; + + gcc_assert (pass->static_pass_number > 0); + gcc_assert (pass_tab); + + VEC_replace (char_ptr, pass_tab, pass->static_pass_number, + (*p)->unique_name); + + return 1; +} + +/* The function traverses NAME_TO_PASS_MAP and creates a pass info + table for dumping purpose. */ + +static void +create_pass_tab (void) +{ + if (!flag_dump_passes) + return; + + VEC_safe_grow_cleared (char_ptr, heap, + pass_tab, passes_by_id_size + 1); + htab_traverse (name_to_pass_map, pass_traverse, NULL); +} + +static bool override_gate_status (struct opt_pass *, tree, bool); + +/* Dump the instantiated name for PASS. IS_ON indicates if PASS + is turned on or not. */ + +static void +dump_one_pass (struct opt_pass *pass, int pass_indent) +{ + int indent = 3 * pass_indent; + const char *pn; + bool is_on, is_really_on; + + is_on = (pass->gate == NULL) ? true : pass->gate(); + is_really_on = override_gate_status (pass, NULL, is_on); + + if (pass->static_pass_number <= 0) + pn = pass->name; + else + pn = VEC_index (char_ptr, pass_tab, pass->static_pass_number); + + fprintf (stderr, "%*s%-40s%*s:%s%s\n", indent, " ", pn, + (15 - indent < 0 ? 0 : 15 - indent), " ", + is_on ? " ON" : " OFF", + ((!is_on) == (!is_really_on) ? "" + : (is_really_on ? " (FORCED_ON)" : " (FORCED_OFF)"))); +} + +/* Dump pass list PASS with indentation INDENT. */ + +static void +dump_pass_list (struct opt_pass *pass, int indent) +{ + do + { + dump_one_pass (pass, indent); + if (pass->sub) + dump_pass_list (pass->sub, indent + 1); + pass = pass->next; + } + while (pass); +} + +/* Dump all optimization passes. */ + +void +dump_passes (void) +{ + create_pass_tab(); + + dump_pass_list (all_lowering_passes, 1); + dump_pass_list (all_small_ipa_passes, 1); + dump_pass_list (all_regular_ipa_passes, 1); + dump_pass_list (all_lto_gen_passes, 1); + dump_pass_list (all_passes, 1); +} + + /* Returns the pass with NAME. */ static struct opt_pass * @@ -513,9 +608,8 @@ get_pass_by_name (const char *name) { struct pass_registry **slot, pr; - gcc_assert (pass_name_tab); pr.unique_name = name; - slot = (struct pass_registry **) htab_find_slot (pass_name_tab, + slot = (struct pass_registry **) htab_find_slot (name_to_pass_map, &pr, NO_INSERT); if (!slot || !*slot) Index: testsuite/gcc.dg/dump-pass.c =================================================================== --- testsuite/gcc.dg/dump-pass.c (revision 0) +++ testsuite/gcc.dg/dump-pass.c (revision 0) @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-passes" } */ + +unsigned res; + +void +foo (unsigned code, int len) +{ + int i; + for (i = 0; i < len; i++) + res |= code & 1; +} + +/* { dg-prune-output ".*" } */