New options to disable/enable any pass for any functions (issue4550056)

Xinliang David Li davidxl@google.com
Wed Jun 1 00:04:00 GMT 2011


Please discard the previous one. This is the right one:

David

On Tue, May 31, 2011 at 5:01 PM, Xinliang David Li <davidxl@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 <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-print3.p
Type: text/x-pascal
Size: 6068 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20110601/82bf8edd/attachment.bin>


More information about the Gcc-patches mailing list