Re: Fwd: RFA: plugin events for ICI

[Richard Guenther <richard dot guenther at gmail dot com>]

On Tue, Dec 1, 2009 at 2:05 PM, Joern Rennecke <amylaar@spamcop.net> wrote:
> Quoting Richard Guenther <richard.guenther@gmail.com>:
>>
>> There is a lot of useful general cleanup mixed with more questionable
>> changes. ?For one, comments like 'Changed for FICI0: ' or
>> 'ICI Event: Substitution of pass manager.' do not belong in GCC
>> source code.
>
> Ok, I'll work on changing these.
> So, where a symbol has been made global for the benefit of ICI, I should
> only say that it has been made global so that plugins can use it, without
> mentioning any plugin in particular?

Yes.

>> I do not particularly like the loop unroll changes (you have a pass
>> start / end hook, why can't you modify the globals from there?).
>
> I suppose I could replace the loop-init.c:rtl_unroll_and_peel_loops
> patch with a PLUGIN_PASS_EXECUTION callback. ?It will be somewhat ugly to
> get the different PLUGIN_PASS_EXECUTION callbacks in the desired order,
> and the globals will be changed after pass execution (there is not actually
> a pass end hook), but I suppose the end result will be the same.
>
> However, this approach could not be used for the loop-unroll.c patches.
> The features exposed and the heuristics manipulation work on a per-loop
> basis.

Still I'd like to see a more general approach for this kind of stuff
(and thus the best is to leave it out of the current patch and
discuss it separately).

>> It smells like very specific to ICI.
>
> Are you referring to comments and identifier names, or do you have
> objections
> to the functionality and/or interface?

Mostly the interface.

>> Likewise I do not like
>>
>> + ?/* ICI Event: Substitution of pass manager. ?*/
>> + ?/* Try calling the event - if not successful, or if the plugin did not
>> + ? ? manipulate passes, fall back on the default pass ordering. ?*/
>> + ?substitute_status = 0;
>> + ?invoke_plugin_callbacks (PLUGIN_ALL_PASSES_EXECUTION,
>> &substitute_status);
>> + ?if (substitute_status == 0)
>> + ? ?execute_pass_list (all_passes);
>> +
>
> Again - is this about the comment, or about the functionality too?
> The functionality is central to the pass reordering in ICI.

It's about the interface.  Please split it out from the patch and
submit it separately so a focused discussion is possible.

For example I don't see what the hook is supposed to do?
It seems it is supposed to be the pass manager itself?
(Btw, why does invoke_plugin_callbacks not return a value,
that would make code way more readable than going through
the passed by reference appearant return value ...)

So, why not instead re-use

invoke_plugin_callbacks (PLUGIN_ALL_PASSES_START, NULL);

to do this?  Or rather, why not have this or another hook
mess with all_passes directly?

Personally I would have expected that execute_pass_list
and execute_one_pass itself become hooks in a pass-manager
hook table and a plugin could replace these.  The current
hooking seems to be glued ontop of things without considering
a more reasonable design...

Thanks,
Richard.