Honza, are you ok with the pass name change? David
On Tue, May 31, 2011 at 2:07 AM, Richard Guenther <richard.guent...@gmail.com> wrote: > On Mon, May 30, 2011 at 11:44 PM, Xinliang David Li <davi...@google.com> > wrote: >> This is the complete patch for pass name fixes (with test case changes). > > This is ok if Honza thinks the profile pass names make more sense this > way. > > Thanks, > Richard. > >> David >> >> >> On Mon, May 30, 2011 at 1: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? >>> >>> 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 >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >