On Tue, Jun 7, 2011 at 8:54 PM, Xinliang David Li <davi...@google.com> wrote:
> Please review the attached two patches.
>
> In the first patch, gate functions are cleaned up. All the per
> function legality checks are moved into the executor and the
> optimization heuristic checks (optimize for size) remain in the
> gators. These allow the the following overriding order:
>
> common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
> heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
> options <--- legality check
>
> Testing under going. Ok for trunk?
It's somewhat odd that you keep the optimize for size/speed checks
in the gate - yes, they also work with a NULL cfun, returning a "default"
(previously they were just optimize_size checks). So probably it
makes sense to keep those in the gates.
I notice that with the patch we will now unconditionally execute
TODOs for those passes that were disabled per function which
will eventually cause some compile-time regression, mostly
restricted to checking enabled builds (which is ok I guess). I
suppose we could allow the execute function to return a special
value that says "I really didn't do anything". OTOH most of the
pass machinery needs some cleanup anyway which can be done
as followup.
The gate cleanup patch looks ok to me, please give others a chance
to comment on this change.
Thanks,
Richard.
> Thanks,
>
> David
>
> On Tue, Jun 7, 2011 at 9:24 AM, Xinliang David Li <davi...@google.com> wrote:
>> Ok -- that sounds good.
>>
>> David
>>
>> On Tue, Jun 7, 2011 at 3:10 AM, Richard Guenther
>> <richard.guent...@gmail.com> wrote:
>>> On Mon, Jun 6, 2011 at 6:00 PM, Xinliang David Li <davi...@google.com>
>>> wrote:
>>>> On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther
>>>> <richard.guent...@gmail.com> wrote:
>>>>> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davi...@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 <davi...@google.com>
>>>>>> wrote:
>>>>>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>>>>>>> <richard.guent...@gmail.com> wrote:
>>>>>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davi...@google.com>
>>>>>>>> wrote:
>>>>>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>>>>>>> <richard.guent...@gmail.com> wrote:
>>>>>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li
>>>>>>>>>> <davi...@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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
