On Wed, Jun 1, 2011 at 2:03 AM, Xinliang David Li <davi...@google.com> wrote: > Please discard the previous one. This is the right one:
See also Honzas comments (on the wrong patch presumably ;)). + if (node) + fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d, decl_uid = %d, cgraph_uid=%d)", + dname, aname, fun->funcdef_no, DECL_UID(fdecl), node->uid); + else + fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d, decl_uid = %d)", + dname, aname, fun->funcdef_no, DECL_UID(fdecl)); + + fprintf (dump_file, "%s\n\n", + node->frequency == NODE_FREQUENCY_HOT you also need to check for node == NULL here. Ok with this (and honzas suggested change for not passing struct function *). Thanks, Richard. > David > > On Tue, May 31, 2011 at 5:01 PM, Xinliang David Li <davi...@google.com> wrote: >> The new patch is attached. The test (c,c++,fortran, java, ada) is on going. >> >> Thanks, >> >> David >> >> On Tue, May 31, 2011 at 9:06 AM, Xinliang David Li <davi...@google.com> >> wrote: >>> On Tue, May 31, 2011 at 2:05 AM, Richard Guenther >>> <richard.guent...@gmail.com> wrote: >>>> On Mon, May 30, 2011 at 10:16 PM, Xinliang David Li <davi...@google.com> >>>> wrote: >>>>> The attached are two simple follow up patches >>>>> >>>>> 1) the first patch does some refactorization on function header >>>>> dumping (with more information printed) >>>>> >>>>> 2) the second patch cleans up some pass names. Part of the cleanup >>>>> results from a previous discussion with Honza -- a) rename >>>>> 'tree_profile_ipa' into 'profile', and make 'ipa_profile' and >>>>> 'profile' into 'profile_estimate'. The rest of cleanups are needed to >>>>> make sure pass names are unique. >>>>> >>>>> Ok for trunk? >>>> >>>> + >>>> +void >>>> +pass_dump_function_header (FILE *dump_file, tree fdecl, struct function >>>> *fun) >>>> >>>> This function needs documentation, the ChangeLog entry misses >>>> the tree-pretty-print.h change. >>>> >>>> +struct function; >>>> >>>> instead of this please include coretypes.h from tree-pretty-print.h >>>> and add the struct function forward declaration there if it isn't already >>>> present. >>>> >>>> You change the output of the header, so please make sure you >>>> have bootstrapped and tested with _all_ languages included >>>> (and also watch for bugreports for target specific bugs). >>>> >>> >>> Ok. >>> >>>> + fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d, uid=%d)", >>>> + dname, aname, fun->funcdef_no, node->uid); >>>> >>>> I see no point in dumping funcdef_no - it wasn't dumped before in >>>> any place. Instead I miss dumping of the DECL_UID and thus >>>> a more specific 'uid', like 'cgraph-uid'. >>> >>> Ok will add decl_uid. Funcdef_no is very useful for debugging FDO >>> coverage mismatch related problems as it is the id used in profile >>> hashing. >>> >>>> >>>> + aname = (IDENTIFIER_POINTER >>>> + (DECL_ASSEMBLER_NAME (fdecl))); >>>> >>>> using DECL_ASSEMBLER_NAME is bad - it might trigger computation >>>> of DECL_ASSEMBLER_NAME which certainly shouldn't be done >>>> only for dumping purposes. Instead do sth like >>>> >>>> if (DECL_ASSEMBLER_NAME_SET_P (fdecl)) >>>> aname = DECL_ASSEMBLER_NAME (fdecl); >>>> else >>>> aname = '<unset-asm-name>'; >>> >>> Ok. >>> >>>> >>>> and please also watch for cgraph_get_node returning NULL. >>>> >>>> Also please call the function dump_function_header instead of >>>> pass_dump_function_header. >>>> >>> >>> Ok. >>> >>> Thanks, >>> >>> David >>>> Please re-post with appropriate changes. >>>> >>>> Thanks, >>>> Richard. >>>> >>>>> Thanks, >>>>> >>>>> David >>>>> >>>>> On Fri, May 27, 2011 at 2:58 AM, Richard Guenther >>>>> <richard.guent...@gmail.com> wrote: >>>>>> On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davi...@google.com> >>>>>> wrote: >>>>>>> The latest version that only exports 2 functions from passes.c. >>>>>> >>>>>> Ok with ... >>>>>> >>>>>> @@ -637,4 +637,8 @@ extern bool first_pass_instance; >>>>>> /* Declare for plugins. */ >>>>>> extern void do_per_function_toporder (void (*) (void *), void *); >>>>>> >>>>>> +extern void disable_pass (const char *); >>>>>> +extern void enable_pass (const char *); >>>>>> +struct function; >>>>>> + >>>>>> >>>>>> struct function forward decl removed. >>>>>> >>>>>> + explicitly_enabled = is_pass_explicitly_enabled (pass, func); >>>>>> + explicitly_disabled = is_pass_explicitly_disabled (pass, func); >>>>>> >>>>>> both functions inlined here and removed. >>>>>> >>>>>> +#define MAX_PASS_ID 512 >>>>>> >>>>>> this removed and instead a VEC_safe_grow_cleared () or VEC_length () >>>>>> before the accesses. >>>>>> >>>>>> +-fenable-ipa-@var{pass} @gol >>>>>> +-fenable-rtl-@var{pass} @gol >>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol >>>>>> +-fenable-tree-@var{pass} @gol >>>>>> +-fenable-tree-@var{pass}=@var{range-list} @gol >>>>>> >>>>>> -fenable-@var{kind}-@var{pass}, etc. >>>>>> >>>>>> +@item -fdisable-@var{ipa|tree|rtl}-@var{pass} >>>>>> +@itemx -fenable-@var{ipa|tree|rtl}-@var{pass} >>>>>> +@itemx -fdisable-@var{tree|rtl}-@var{pass}=@var{range-list} >>>>>> +@itemx -fenable-@var{tree|rtl}-@var{pass}=@var{range-list} >>>>>> >>>>>> likewise. >>>>>> >>>>>> Thanks, >>>>>> Richard. >>>>>> >>>>>>> David >>>>>>> >>>>>>> On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davi...@google.com> >>>>>>> wrote: >>>>>>>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther >>>>>>>> <richard.guent...@gmail.com> wrote: >>>>>>>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers >>>>>>>>> <jos...@codesourcery.com> wrote: >>>>>>>>>> On Wed, 25 May 2011, Xinliang David Li wrote: >>>>>>>>>> >>>>>>>>>>> Ping. The link to the message: >>>>>>>>>>> >>>>>>>>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html >>>>>>>>>> >>>>>>>>>> I don't consider this an option handling patch. Patches adding >>>>>>>>>> whole new >>>>>>>>>> features involving new options should be reviewed by maintainers for >>>>>>>>>> the >>>>>>>>>> part of the compiler relevant to those features (since there isn't a >>>>>>>>>> pass >>>>>>>>>> manager maintainer, I guess that means middle-end). >>>>>>>>> >>>>>>>>> Hmm, I suppose then you reviewed the option handling parts and they >>>>>>>>> are ok? Those globbing options always cause headache to me. >>>>>>>>> >>>>>>>>> +-fenable-ipa-@var{pass} @gol >>>>>>>>> +-fenable-rtl-@var{pass} @gol >>>>>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol >>>>>>>>> +-fenable-tree-@var{pass} @gol >>>>>>>>> >>>>>>>>> so, no -fenable-tree-@var{pass}=@var{range-list}? >>>>>>>>> >>>>>>>> >>>>>>>> Missed. Will add. >>>>>>>> >>>>>>>> >>>>>>>>> Does the pass name match 1:1 with the dump file name? In which >>>>>>>>> case >>>>>>>> >>>>>>>> Yes. >>>>>>>> >>>>>>>>> >>>>>>>>> +Disable ipa pass @var{pass}. @var{pass} is the pass name. If the same >>>>>>>>> pass is statically invoked in the compiler multiple times, the pass >>>>>>>>> name should be appended with a sequential number starting from 1. >>>>>>>>> >>>>>>>>> isn't true as passes that are invoked only a single time lack the >>>>>>>>> number >>>>>>>>> suffix (yes, I'd really like that to be changed ...) >>>>>>>> >>>>>>>> Yes, pass with single static instance does not need number suffix. >>>>>>>> >>>>>>>>> >>>>>>>>> Please break likes also in .texi files and stick to 80 columns. >>>>>>>> >>>>>>>> Done. >>>>>>>> >>>>>>>>> Please >>>>>>>>> document that these options are debugging options and regular >>>>>>>>> options for enabling/disabling passes should be used. I would suggest >>>>>>>>> to group documentation differently and document -fenable-* and >>>>>>>>> -fdisable-*, thus, >>>>>>>>> >>>>>>>>> + -fdisable-@var{kind}-@var{pass} >>>>>>>>> + -fenable-@var{kind}-@var{pass} >>>>>>>>> >>>>>>>>> Even in .texi files please two spaces after a full-stop. >>>>>>>> >>>>>>>> Done >>>>>>>> >>>>>>>>> >>>>>>>>> +extern void enable_disable_pass (const char *, bool); >>>>>>>>> >>>>>>>>> I'd rather have both enable_pass and disable_pass ;) >>>>>>>> >>>>>>>> Ok. >>>>>>>> >>>>>>>>> >>>>>>>>> +struct function; >>>>>>>>> +extern void pass_dump_function_header (FILE *, tree, struct function >>>>>>>>> *); >>>>>>>>> >>>>>>>>> that's odd and probably should be split out, the function should >>>>>>>>> maybe reside in tree-pretty-print.c. >>>>>>>> >>>>>>>> Ok. >>>>>>>> >>>>>>>>> >>>>>>>>> Index: tree-ssa-loop-ivopts.c >>>>>>>>> =================================================================== >>>>>>>>> --- tree-ssa-loop-ivopts.c (revision 173837) >>>>>>>>> +++ tree-ssa-loop-ivopts.c (working copy) >>>>>>>>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d >>>>>>>>> >>>>>>>>> well - doesn't belong here ;) >>>>>>>> >>>>>>>> Sorry -- many things in the same client -- not needed with your latest >>>>>>>> change anyway. >>>>>>>> >>>>>>>>> >>>>>>>>> +static hashval_t >>>>>>>>> +passr_hash (const void *p) >>>>>>>>> +{ >>>>>>>>> + const struct pass_registry *const s = (const struct pass_registry >>>>>>>>> *const) p; >>>>>>>>> + return htab_hash_string (s->unique_name); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +/* Hash equal function */ >>>>>>>>> + >>>>>>>>> +static int >>>>>>>>> +passr_eq (const void *p1, const void *p2) >>>>>>>>> +{ >>>>>>>>> + const struct pass_registry *const s1 = (const struct pass_registry >>>>>>>>> *const) p1; >>>>>>>>> + const struct pass_registry *const s2 = (const struct pass_registry >>>>>>>>> *const) p2; >>>>>>>>> + >>>>>>>>> + return !strcmp (s1->unique_name, s2->unique_name); >>>>>>>>> +} >>>>>>>>> >>>>>>>>> you can use htab_hash_string and strcmp directly, no need for these >>>>>>>>> wrappers. >>>>>>>> >>>>>>>> The hashtable entry is not string in this case. It is pass_registry, >>>>>>>> thus the wrapper. >>>>>>>> >>>>>>>>> >>>>>>>>> +void >>>>>>>>> +register_pass_name (struct opt_pass *pass, const char *name) >>>>>>>>> >>>>>>>>> doesn't seem to need exporting, so don't and make it static. >>>>>>>> >>>>>>>> Done. >>>>>>>> >>>>>>>>> >>>>>>>>> + if (!pass_name_tab) >>>>>>>>> + pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL); >>>>>>>>> >>>>>>>>> see above, the initial size should be larger - we have 223 passes at >>>>>>>>> the >>>>>>>>> moment, so use 256. >>>>>>>> >>>>>>>> Done. >>>>>>>> >>>>>>>>> >>>>>>>>> + else >>>>>>>>> + return; /* Ignore plugin passes. */ >>>>>>>>> >>>>>>>>> ? You mean they might clash? >>>>>>>> >>>>>>>> Yes, name clash. >>>>>>>> >>>>>>>>> >>>>>>>>> +struct opt_pass * >>>>>>>>> +get_pass_by_name (const char *name) >>>>>>>>> >>>>>>>>> doesn't need exporting either. >>>>>>>> >>>>>>>> Done. >>>>>>>> >>>>>>>>> >>>>>>>>> + if (is_enable) >>>>>>>>> + error ("unrecognized option -fenable"); >>>>>>>>> + else >>>>>>>>> + error ("unrecognized option -fdisable"); >>>>>>>>> >>>>>>>>> I think that should be fatal_error - Joseph? >>>>>>>>> >>>>>>>>> + if (is_enable) >>>>>>>>> + error ("unknown pass %s specified in -fenable", phase_name); >>>>>>>>> + else >>>>>>>>> + error ("unknown pass %s specified in -fdisble", phase_name); >>>>>>>>> >>>>>>>>> likewise. >>>>>>>>> >>>>>>>>> + if (!enabled_pass_uid_range_tab) >>>>>>>>> + enabled_pass_uid_range_tab = htab_create (10, pass_hash, >>>>>>>>> pass_eq, NULL); >>>>>>>>> >>>>>>>>> instead of using a hashtable here please use a VEC indexed by >>>>>>>>> the static_pass_number which shoud speed up >>>>>>>> >>>>>>>> Ok. The reason I did not use it is because in most of the cases, the >>>>>>>> htab will be very small -- it is determined by the number of passes >>>>>>>> specified in the command line, while the VEC requires allocating const >>>>>>>> size array. Not an issue as long as by default the array is not >>>>>>>> allocated. >>>>>>>> >>>>>>>>> >>>>>>>>> +static bool >>>>>>>>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass, >>>>>>>>> + tree func, htab_t tab) >>>>>>>>> +{ >>>>>>>>> + struct uid_range **slot, *range, key; >>>>>>>>> + int cgraph_uid; >>>>>>>>> + >>>>>>>>> + if (!tab) >>>>>>>>> + return false; >>>>>>>>> + >>>>>>>>> + key.pass = pass; >>>>>>>>> + slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT); >>>>>>>>> + if (!slot || !*slot) >>>>>>>>> + return false; >>>>>>>>> >>>>>>>>> and simplify the code quite a bit. >>>>>>>>> >>>>>>>>> + cgraph_uid = func ? cgraph_get_node (func)->uid : 0; >>>>>>>>> >>>>>>>>> note that cgraph uids are recycled, so it might not be the best idea >>>>>>>>> to use them as discriminator (though I don't have a good idea how >>>>>>>>> to represent ranges without them). >>>>>>>> >>>>>>>> Yes. It is not a big problem as the blind search does not need to know >>>>>>>> the id->name mapping. Once the id s found, it can be easily discovered >>>>>>>> via dump. >>>>>>>> >>>>>>>>> >>>>>>>>> + explicitly_enabled = is_pass_explicitly_enabled (pass, >>>>>>>>> current_function_decl); >>>>>>>>> + explicitly_disabled = is_pass_explicitly_disabled (pass, >>>>>>>>> current_function_decl); >>>>>>>>> + >>>>>>>>> current_pass = pass; >>>>>>>>> >>>>>>>>> /* Check whether gate check should be avoided. >>>>>>>>> User controls the value of the gate through the parameter >>>>>>>>> "gate_status". */ >>>>>>>>> gate_status = (pass->gate == NULL) ? true : pass->gate(); >>>>>>>>> + gate_status = !explicitly_disabled && (gate_status || >>>>>>>>> explicitly_enabled); >>>>>>>>> >>>>>>>>> so explicitly disabling wins over explicit enabling ;) I think this >>>>>>>>> implementation detail and the fact that you always query both >>>>>>>>> hints at that the interface should be like >>>>>>>>> >>>>>>>>> gate_status = override_gate_status (pass, current_function_decl, >>>>>>>>> gate_status); >>>>>>>> >>>>>>>> Done. >>>>>>>> >>>>>>>>> >>>>>>>>> instead. >>>>>>>>> >>>>>>>>> Thus, please split out the function header dumping changes and rework >>>>>>>>> the rest of the patch as suggested. >>>>>>>> >>>>>>>> Split out. The new patch is attached. >>>>>>>> >>>>>>>> Ok after testing is done? >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> David >>>>>>>> >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Richard. >>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Joseph S. Myers >>>>>>>>>> jos...@codesourcery.com >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >