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

[Richard Guenther <richard.guenther@gmail.com>]

On Wed, Jun 1, 2011 at 2:03 AM, Xinliang David Li <davidxl@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 <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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>