Re: Fwd: RFA: plugin events for ICI

[Joern Rennecke <amylaar@spamcop.net>]

Quoting Basile STARYNKEVITCH <basile@starynkevitch.net>:
> --- gcc/gcc/doc/plugins.texi	2009-11-29 17:42:32.565901921 +0000
> +++ gcc-ici-2/gcc/doc/plugins.texi	2009-11-30 16:35:59.804653957 +0000
> @@ -156,18 +156,48 @@ enum plugin_event
>     PLUGIN_ATTRIBUTES,            /* Called during attribute registration */
>     PLUGIN_START_UNIT,            /* Called before processing a   
> translation unit.  */
>     PLUGIN_PRAGMAS,	        /* Called during pragma registration. */
> -  PLUGIN_EVENT_LAST             /* Dummy event used for indexing callback
> +  /* Called to allow inspection and/or modification of unroll   
> parameters.  */
> +  PLUGIN_UNROLL_PARAMETER_HANDLER:
>
> Don't put colons here. You should have exactly the same code as in   
> gcc/gcc-plugin.h

Actually, there is only an include.  The enumeration is in plugins.def,
and I had cut & pasted the list of new plugins from a switch statement.
But you are right, unless I want to change the entire list, I should put
a comma instead of a semicolon after each enumerator.

> +  PLUGIN_OVERRIDE_GATE:
> +  /* Called before executing a pass.  */
>
> Why?

The adapt plugin wants to enable passes independently of the original
gate function.  E.g. with milepost this can be driven by machine learning.

> +  PLUGIN_PASS_EXECUTION:
> +  /* Called before executing subpasses of a GIMPLE_PASS in
> +     execute_ipa_pass_list.  */
>
> Why?

This allows recording of the passes that are executed.

> @@ -1491,9 +1500,21 @@ execute_one_pass (struct opt_pass *pass)
>
>     current_pass = pass;
>
> -  /* See if we're supposed to run this pass.  */
> -  if (pass->gate && !pass->gate ())
> +  /* 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();
> +
> +  /* Override gate with plugin.  */
> +  invoke_plugin_callbacks (PLUGIN_OVERRIDE_GATE, &gate_status);
> +
>
> This something I don't fully understand. Maybe it is just lacking   
> documentation.
>
> What is the purpose of invoking a callback here? Plugins may add   
> their own passes.
> I would understand better if you would suggest to dynamically add   
> gate functions into existing passes (in addition of
> their own one).

As explained before, this allows the adapt plugin to override the gate
decision.  This event was originally called "avoid_gate" since it was
only used to make the pass run onconditionally once you got to that point
in the code; however, I thought that was a misnomer, because the gate function
is not avoided, and the callback could be used to disable passes that
are usually enabled as well as the current usage of enabling passes
that are ordinarily disabled.

> If it is your mechanism for reaordering passes, you could document   
> it better (perhaps in plugins.texi)

There are AFAICT nine different callbacks involved in reordering passes,
and some also double up for other purposes.  Only the  
PLUGIN_ALL_PASSES_EXECUTION / PLUGIN_ALL_IPA_PASSES_EXECUTION events
have really a strong affinity to pass reordering, the other ones can
lend themselves to all kinds of other purposes.

> And your patch contain also for gcc/tree-pass.h
>
> +/* Declare for plugins like ICI.  */
> +extern void do_per_function_toporder (void (*) (void *), void *);
> +
>
> The comment is not at all explanatory. Don't mention ICI or any   
> specific plugin, but explain the reason behind the
> function. Please write several sentences, explaining what is the   
> first function argument, and what is the second
> (perhaps some client data?).

The function is explained in the passes.c, were it has been defined all along.
The change is merely to make a formerly static function globally visible.
As this is not needed by any code inside GCC, a comment is needed to explain
why this function is not (any longer) static.