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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
