This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: New options to disable/enable any pass for any functions (issue4550056)
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
>>>>
>>>
>>
>