[06/11] Rewrite how instances of passes are cloned

gcc/

	Rewrite how instances of passes are cloned to remove assumptions
	about their sizes (thus allowing pass subclasses to have
	additional data fields, albeit non-GC-managed ones at this point).

	* passes.c (make_pass_instance): Now that passes have clone
	methods, rewrite this function to eliminate XNEW and memcpy
	calls that used hardcoded sizes.  Since this function no longer
	creates pass instances, rename it to...
	(add_pass_instance): ...this.  Document the old way that passes
	were numbered and flagged, and rework this function to continue
	using it.
	(next_pass_1): Add an initial_pass argument for use by
	add_pass_instance.
	(position_pass): When adding multiple instances of a pass, use
	the pass's clone method, rather than relying on the XNEW/memcpy
	within the former make_pass_instance (now add_pass_instance).
	(pipeline::pipeline): When invoking next_pass_1, also supply the
	initial instance of the current pass within the pipeline.
---
 gcc/passes.c | 92 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 55 insertions(+), 37 deletions(-)

This patch does more than just remove the hardcoded assumptions about
pass sizes - as noted in
http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00041.html
it also is needed to ensure that per-pass dumpfiles get the correct
switch names.

So the short version is that patch 6 is needed for patches 3-5 to work,
ensuring that the command-line switches for the per-pass dumpfiles don't
change.

Here's the long version:

AIUI, the "static_pass_number" field is used in two different ways:
during pass creation for tracking instance counts within a pass family,
and after that, for tracking the index within the entire pipeline of
passes (the pass id).

In the current implementation, during pass creation, the first time a
pass is seen, the static_pass_number is set to -1.  On subsequent
instances of the pass, the pass is copied, and that number decremented,
so that if you have a pass with 4 instances, their static_pass_number
fields will be -4, 2, 3, 4 (note how the initial one is negative and
gives the count).

There is this logic in register_one_dump_file to determine the suffix
used for per-pass-instance dumpfiles:

  if (pass->static_pass_number != -1)
    sprintf (num, "%d", ((int) pass->static_pass_number < 0
			 ? 1 : pass->static_pass_number));

This is then used by dump_register, and affects the "swtch" field of the
relevant entry within extra_dump_files, so that unique passes have no
num in their dumpfile switch, and multiinstance passes have dumpfile
switches with numeric suffices.

This works because the code is always referring to the same pass struct,
and thus the changes to say pass_copy_prop.static_pass_number are seen
and affect later uses.

(Later on in pass initialization, the static_pass_number is overridden
and becomes an id for the pass within the entire pipeline, rather than
just within its category - see the notes in the comment in the patch
below).

In my patch series, with just patches 3 through 5, we're instead making
separate calls to e.g. make_pass_copy_prop, and each instance gets an
initial static_pass_number of 0, which is the changed to -1 by
make_pass_instance (as if it were always the first instance) and hence
the logic above in register_one_dump_file fails, and thus the dump file
of each pass erroneously has no numeric suffix in its dumpfile switch,
as if every pass were a unique instance of its kind.

With patch 6, the new function add_pass_instance sets up
static_pass_number on the new pass instances, mimicking the behavior of
the old code, and so the logic above in register_one_dump_file gets the
dump file switch names correct.

I reviewed this in gdb using this Python one-liner to see the ->swtch
field of each element of the extra_dump_files array:
(gdb) python print \
[gdb.parse_and_eval('extra_dump_files')[i]['swtch'].string() for i in
range(gdb.parse_and_eval('extra_dump_files_in_use'))]

and without patch 6, all of the extra dumpfiles for the 8 copy_prop
instances erroneously share the switch "tree-copyprop".

With patch 6, they correctly have switches "tree-copyprop1" through
"tree-copyprop8".

Much of this is covered by the comment that the patch adds, but I should
have spelled things out more in the initial posting of the patch; sorry.

OK for trunk?  (on top of the other patches, of course; see notes in 
http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00041.html in how I've
tested this).

On Fri, 2013-07-26 at 11:04 -0400, David Malcolm wrote:
> gcc/
> 
> 	Rewrite how instances of passes are cloned to remove assumptions
> 	about their sizes (thus allowing pass subclasses to have
> 	additional data fields, albeit non-GC-managed ones at this point).
> 
> 	* passes.c (make_pass_instance): Now that passes have clone
> 	methods, rewrite this function to eliminate XNEW and memcpy
> 	calls that used hardcoded sizes.  Since this function no longer
> 	creates pass instances, rename it to...
> 	(add_pass_instance): ...this.  Document the old way that passes
> 	were numbered and flagged, and rework this function to continue
> 	using it.
> 	(next_pass_1): Add an initial_pass argument for use by
> 	add_pass_instance.
> 	(position_pass): When adding multiple instances of a pass, use
> 	the pass's clone method, rather than relying on the XNEW/memcpy
> 	within the former make_pass_instance (now add_pass_instance).
> 	(pipeline::pipeline): When invoking next_pass_1, also supply the
> 	initial instance of the current pass within the pipeline.
> ---
>  gcc/passes.c | 92 ++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 55 insertions(+), 37 deletions(-)
> 
> diff --git a/gcc/passes.c b/gcc/passes.c
> index ead41a8..ce5cdeb 100644
> --- a/gcc/passes.c
> +++ b/gcc/passes.c
> @@ -1167,68 +1167,77 @@ is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>    return false;
>  }
>  
> -/* Look at the static_pass_number and duplicate the pass
> -   if it is already added to a list. */
>  
> -static struct opt_pass *
> -make_pass_instance (struct opt_pass *pass, bool track_duplicates)
> -{
> -  /* A nonzero static_pass_number indicates that the
> -     pass is already in the list.  */
> -  if (pass->static_pass_number)
> -    {
> -      struct opt_pass *new_pass;
> +/* Update static_pass_number for passes (and the flag
> +   TODO_mark_first_instance).
>  
> -      if (pass->type == GIMPLE_PASS
> -          || pass->type == RTL_PASS
> -          || pass->type == SIMPLE_IPA_PASS)
> -        {
> -          new_pass = XNEW (struct opt_pass);
> -          memcpy (new_pass, pass, sizeof (struct opt_pass));
> -        }
> -      else if (pass->type == IPA_PASS)
> -        {
> -          new_pass = (struct opt_pass *)XNEW (struct ipa_opt_pass_d);
> -          memcpy (new_pass, pass, sizeof (struct ipa_opt_pass_d));
> -        }
> -      else
> -        gcc_unreachable ();
> +   Passes are constructed with static_pass_number preinitialized to 0
> +
> +   This field is used in two different ways: initially as instance numbers
> +   of their kind, and then as ids within the entire pipeline.
> +
> +   Within pipeline::pipeline:
> +
> +   * In add_pass_instance(), as called by next_pass_1 in
> +     NEXT_PASS in init_optimization_passes
>  
> -      new_pass->next = NULL;
> +   * When the initial instance of a pass within a pipeline is seen,
> +     it is flagged, and its static_pass_number is set to -1
>  
> +   * On subsequent times that it is seen, the static pass number
> +     is decremented each time, so that if there are e.g. 4 dups,
> +     they have static_pass_number -4, 2, 3, 4 respectively (note
> +     how the initial one is negative and gives the count); these
> +     can be thought of as instance numbers of the specific pass
> +
> +   * Within the register_dump_files () traversal, set_pass_for_id()
> +     is called on each pass, using these instance numbers to create
> +     dumpfile switches, and then overwriting them with a pass id,
> +     which are global to the whole pass pipeline (based on
> +     (TDI_end + current value of extra_dump_files_in_use) )  */
> +
> +static void
> +add_pass_instance (struct opt_pass *new_pass, bool track_duplicates,
> +		   opt_pass *initial_pass)
> +{
> +  /* Are we dealing with the first pass of its kind, or a clone?  */
> +  if (new_pass != initial_pass)
> +    {
> +      /* We're dealing with a clone.  */
>        new_pass->todo_flags_start &= ~TODO_mark_first_instance;
>  
>        /* Indicate to register_dump_files that this pass has duplicates,
>           and so it should rename the dump file.  The first instance will
>           be -1, and be number of duplicates = -static_pass_number - 1.
>           Subsequent instances will be > 0 and just the duplicate number.  */
> -      if ((pass->name && pass->name[0] != '*') || track_duplicates)
> +      if ((new_pass->name && new_pass->name[0] != '*') || track_duplicates)
>          {
> -          pass->static_pass_number -= 1;
> -          new_pass->static_pass_number = -pass->static_pass_number;
> +          initial_pass->static_pass_number -= 1;
> +          new_pass->static_pass_number = -initial_pass->static_pass_number;
>  	}
> -      return new_pass;
>      }
>    else
>      {
> -      pass->todo_flags_start |= TODO_mark_first_instance;
> -      pass->static_pass_number = -1;
> +      /* We're dealing with the first pass of its kind.  */
> +      new_pass->todo_flags_start |= TODO_mark_first_instance;
> +      new_pass->static_pass_number = -1;
>  
> -      invoke_plugin_callbacks (PLUGIN_NEW_PASS, pass);
> +      invoke_plugin_callbacks (PLUGIN_NEW_PASS, new_pass);
>      }
> -  return pass;
>  }
>  
>  /* Add a pass to the pass list. Duplicate the pass if it's already
>     in the list.  */
>  
>  static struct opt_pass **
> -next_pass_1 (struct opt_pass **list, struct opt_pass *pass)
> +next_pass_1 (struct opt_pass **list, struct opt_pass *pass,
> +	     struct opt_pass *initial_pass)
>  {
>    /* Every pass should have a name so that plugins can refer to them.  */
>    gcc_assert (pass->name != NULL);
>  
> -  *list = make_pass_instance (pass, false);
> +  add_pass_instance (pass, false, initial_pass);
> +  *list = pass;
>  
>    return &(*list)->next;
>  }
> @@ -1278,7 +1287,16 @@ position_pass (struct register_pass_info *new_pass_info,
>            struct opt_pass *new_pass;
>            struct pass_list_node *new_pass_node;
>  
> -	  new_pass = make_pass_instance (new_pass_info->pass, true);
> +	  if (new_pass_info->ref_pass_instance_number == 0)
> +	    {
> +	      new_pass = new_pass_info->pass->clone ();
> +	      add_pass_instance (new_pass, true, new_pass_info->pass);
> +	    }
> +	  else
> +	    {
> +	      new_pass = new_pass_info->pass;
> +	      add_pass_instance (new_pass, true, new_pass);
> +	    }
>  
>            /* Insert the new pass instance based on the positioning op.  */
>            switch (new_pass_info->pos_op)
> @@ -1477,7 +1495,7 @@ pipeline::pipeline (context *ctxt)
>          gcc_assert (PASS ## _1);                 \
>          PASS ## _ ## NUM = PASS ## _1->clone (); \
>        }                                          \
> -    p = next_pass_1 (p, PASS ## _ ## NUM);  \
> +    p = next_pass_1 (p, PASS ## _ ## NUM, PASS ## _1);  \
>    } while (0)
>  
>  #define TERMINATE_PASS_LIST() \

On Thu, 2013-08-01 at 13:55 -0400, David Malcolm wrote:
[...snip...]
> OK for trunk?  (on top of the other patches, of course; see notes in 
> http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00041.html in how I've
> tested this).

...with "pipeline" renamed to "pass_manager", of course.

On 08/01/2013 07:55 AM, David Malcolm wrote:
> On Fri, 2013-07-26 at 11:04 -0400, David Malcolm wrote:
>> > gcc/
>> > 
>> > 	Rewrite how instances of passes are cloned to remove assumptions
>> > 	about their sizes (thus allowing pass subclasses to have
>> > 	additional data fields, albeit non-GC-managed ones at this point).
>> > 
>> > 	* passes.c (make_pass_instance): Now that passes have clone
>> > 	methods, rewrite this function to eliminate XNEW and memcpy
>> > 	calls that used hardcoded sizes.  Since this function no longer
>> > 	creates pass instances, rename it to...
>> > 	(add_pass_instance): ...this.  Document the old way that passes
>> > 	were numbered and flagged, and rework this function to continue
>> > 	using it.
>> > 	(next_pass_1): Add an initial_pass argument for use by
>> > 	add_pass_instance.
>> > 	(position_pass): When adding multiple instances of a pass, use
>> > 	the pass's clone method, rather than relying on the XNEW/memcpy
>> > 	within the former make_pass_instance (now add_pass_instance).
>> > 	(pipeline::pipeline): When invoking next_pass_1, also supply the
>> > 	initial instance of the current pass within the pipeline.

Ok (with the pass_manager change).


r~