Re: -fdump-passes -fenable-xxx=func_name_list

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

On Mon, Jun 6, 2011 at 6:00 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> This is the version of the patch that walks through pass lists.
>>>
>>> Ok with this one?
>>
>> +/* Dump all optimization passes.  */
>> +
>> +void
>> +dump_passes (void)
>> +{
>> +  struct cgraph_node *n, *node = NULL;
>> +  tree save_fndecl = current_function_decl;
>> +
>> +  fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
>>
>> this isn't accurate info - cloning can cause more cgraph nodes to
>> appear (it also looks completely unrelated to dump_passes ...).
>> Please drop it.
>
> Ok.
>
>
>>
>> +  create_pass_tab();
>> +  gcc_assert (pass_tab);
>>
>> you have quite many asserts of this kind - we don't want them when
>> the previous stmt as in this case indicates everything is ok.
>
> ok.
>
>>
>> +  push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>>
>> this has side-effects, I'm not sure we want this here.  Why do you
>> need it?  Probably because of
>>
>> +  is_really_on = override_gate_status (pass, current_function_decl, is_on);
>>
>> ?  But that is dependent on the function given which should have no
>> effect (unless it is overridden globally in which case override_gate_status
>> and friends should deal with a NULL cfun).
>
> As we discussed, currently some pass gate functions depend on per node
> information -- those checks need to be pushed into execute functions.
> I would like to clean those up later -- at which time, the push/pop
> can be removed.

I'd like to do it the other way around, first clean up the gate functions then
drop in this patch without the cfun push/pop.  The revised patch looks ok
to me with the cfun push/pop removed.

Thanks,
Richard.

>>
>> I don't understand why you need another table mapping pass to name
>> when pass->name is available and the info is trivially re-constructible.
>
> This is needed as the pass->name is not the canonicalized name (i.e.,
> not with number suffix etc), so the extra mapping from id to
> normalized name is needed.
>
> Thanks,
>
> David
>
>>
>> Thanks,
>> Richard.
>>
>>> David
>>>
>>> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>>>> configuration. The sample output is attached.  There is one
>>>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>>>> note registered thus they are not listed. They are not important as
>>>>>>>> they can not be turned on/off anyway.
>>>>>>>>
>>>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>>>> of function assembler names to be specified.
>>>>>>>>
>>>>>>>> Ok for trunk?
>>>>>>>
>>>>>>> Please split the patch.
>>>>>>>
>>>>>>> I'm not too happy how you dump the pass configuration.  Why not simply,
>>>>>>> at a _single_ place, walk the pass tree?  Instead of doing pieces of it
>>>>>>> at pass execution time when it's not already dumped - that really looks
>>>>>>> gross.
>>>>>>
>>>>>> Yes, that was the original plan -- but it has problems
>>>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>>>> frequently -- it can be a long term maintanance burden;
>>>>>> 2) the centralized dumper needs to be done after option processing
>>>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>>>
>>>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>>>> to do the dumping and tracking indentation.
>>>>>
>>>>> Well, if you have a CU that is empty or optimized to nothing at some point
>>>>> you will not get a complete pass list.  I suppose optimize attributes might
>>>>> also confuse output.  Your solution might not be that intrusive
>>>>> but it is still ugly.  I don't see 1) as an issue, for 2) you can just call the
>>>>> dumping from toplev_main before calling do_compile (), 3) gate functions
>>>>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>>>>> your option summary output will be bogus anyway.
>>>>>
>>>>> So - what is the output intended for if it isn't reliable?
>>>>
>>>> This needs to be cleaned up at some point -- the gate function should
>>>> behave the same for all functions and per-function decisions need to
>>>> be pushed down to the executor body.  I will try to rework the patch
>>>> as you suggested to see if there are problems.
>>>>
>>>> David
>>>>
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>>>
>>>>>>> The documentation should also link this option to the -fenable/disable
>>>>>>> options as obviously the pass names in that dump are those to be
>>>>>>> used for those flags (and not readily available anywhere else).
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>>>
>>>>>>> I also think that it would be way more useful to note in the individual
>>>>>>> dump files the functions (at the place they would usually appear) that
>>>>>>> have the pass explicitly enabled/disabled.
>>>>>>
>>>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>>>> explicitly disabled.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> Richard.
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>