New options to disable/enable any pass for any functions (issue4550056)
Xinliang David Li
davidxl@google.com
Wed Jun 1 00:01:00 GMT 2011
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 <davidxl@google.com> wrote:
> On Tue, May 31, 2011 at 2:05 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Mon, May 30, 2011 at 10:16 PM, Xinliang David Li <davidxl@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.guenther@gmail.com> wrote:
>>>> On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davidxl@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 <davidxl@google.com> wrote:
>>>>>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>>>>>>> <joseph@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
>>>>>>>> joseph@codesourcery.com
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: header-print2.p
Type: text/x-pascal
Size: 25899 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20110601/381c71b7/attachment.bin>
More information about the Gcc-patches
mailing list