This is the complete patch for pass name fixes (with test case changes). David
On Mon, May 30, 2011 at 1:16 PM, Xinliang David Li <davi...@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?
>
> Thanks,
>
> David
>
> On Fri, May 27, 2011 at 2:58 AM, Richard Guenther
> <richard.guent...@gmail.com> wrote:
>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>
pass-name-cleanup.p
Description: Binary data
