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