[2/2] Replace the modified flag with AA-queries

Hi,

indirect inlining was written before we had AA-info at our disposal in
the "early" passes and so when confronted with the necessity to
guarantee that a member pointer parameter was not changed before it
was used I have resorted to requiring it is never modified in the
whole function.

With alias analysis information this is no longer necessary.  This
code therefore does away with the flag and uses AA instead.  To big
extent I have copied the clobbering identification code from IPA-SRA
and hope I got it correct.

I added a new testcase checking that we so not indirect-inline when we
shouldn't because a member-pointer parameter is modified before being
passed on.

I am aware that there are no hunks removing testcase
gcc.dg/ipa/modif-1.c in the patch, that is because I generate my
patches with quilt rather than svn.  I will remember to svn-delete the
file.

I have bootstrapped and tested this patch (on top of the previous one)
on x86_64-linux without any issues,  OK for trunk?

Thanks,

Martin


2010-06-18  Martin Jambor  <mjambor@suse.cz>

	* ipa-prop.h (struct ipa_param_descriptor): Remove the modified flag.
	(ipa_is_param_modified): Removed.
	* ipa-prop.c (visit_store_addr_for_mod_analysis): Do not set the
	modified flag.
	(mark_modified): New function.
	(is_parm_modified_at_call): Likewise.
	(compute_pass_through_member_ptrs): Use is_parm_modified_at_call
	instead of ipa_is_param_modified.
	(ipa_analyze_indirect_call_uses): Likewise.
	(ipa_print_node_params): Do not dump the modified flag.
	(ipa_write_node_info): Do not stream the modified flag.
	(ipa_read_node_info): Likewise.

	* testsuite/g++.dg/ipa/iinline-2.C: New test.
	* testsuite/gcc.dg/ipa/modif-1.c: Removed.

On Mon, 21 Jun 2010, Martin Jambor wrote:

> Hi,
> 
> indirect inlining was written before we had AA-info at our disposal in
> the "early" passes and so when confronted with the necessity to
> guarantee that a member pointer parameter was not changed before it
> was used I have resorted to requiring it is never modified in the
> whole function.
> 
> With alias analysis information this is no longer necessary.  This
> code therefore does away with the flag and uses AA instead.  To big
> extent I have copied the clobbering identification code from IPA-SRA
> and hope I got it correct.
> 
> I added a new testcase checking that we so not indirect-inline when we
> shouldn't because a member-pointer parameter is modified before being
> passed on.
> 
> I am aware that there are no hunks removing testcase
> gcc.dg/ipa/modif-1.c in the patch, that is because I generate my
> patches with quilt rather than svn.  I will remember to svn-delete the
> file.
> 
> I have bootstrapped and tested this patch (on top of the previous one)
> on x86_64-linux without any issues,  OK for trunk?
> 
> Thanks,
> 
> Martin
> 
> 
> 2010-06-18  Martin Jambor  <mjambor@suse.cz>
> 
> 	* ipa-prop.h (struct ipa_param_descriptor): Remove the modified flag.
> 	(ipa_is_param_modified): Removed.
> 	* ipa-prop.c (visit_store_addr_for_mod_analysis): Do not set the
> 	modified flag.
> 	(mark_modified): New function.
> 	(is_parm_modified_at_call): Likewise.
> 	(compute_pass_through_member_ptrs): Use is_parm_modified_at_call
> 	instead of ipa_is_param_modified.
> 	(ipa_analyze_indirect_call_uses): Likewise.
> 	(ipa_print_node_params): Do not dump the modified flag.
> 	(ipa_write_node_info): Do not stream the modified flag.
> 	(ipa_read_node_info): Likewise.
> 
> 	* testsuite/g++.dg/ipa/iinline-2.C: New test.
> 	* testsuite/gcc.dg/ipa/modif-1.c: Removed.
> 
> Index: icln/gcc/ipa-prop.c
> ===================================================================
> --- icln.orig/gcc/ipa-prop.c	2010-06-20 18:23:13.000000000 +0200
> +++ icln/gcc/ipa-prop.c	2010-06-20 18:24:32.000000000 +0200
> @@ -212,7 +212,6 @@ visit_store_addr_for_mod_analysis (gimpl
>      {
>        int index = ipa_get_param_decl_index (info, op);
>        gcc_assert (index >= 0);
> -      info->params[index].modified = true;
>        info->params[index].used = true;
>      }
>  
> @@ -707,6 +706,34 @@ type_like_member_ptr_p (tree type, tree
>    return true;
>  }
>  
> +/* Callback of walk_aliased_vdefs.  Flags that it has been invoked to the
> +   boolean variable pointed to by DATA.  */
> +
> +static bool
> +mark_modified (ao_ref *ao ATTRIBUTE_UNUSED, tree vdef ATTRIBUTE_UNUSED,
> +		     void *data)
> +{
> +  bool *b = (bool *) data;
> +  *b = true;
> +  return true;
> +}
> +
> +/* Return true if the formal parameter PARM might have been modified in this
> +   function before reaching the statement CALL.  */
> +
> +static bool
> +is_parm_modified_at_call (gimple call, tree parm)
> +{
> +  bool modified = false;
> +  ao_ref refd;
> +
> +  ao_ref_init (&refd, parm);
> +  walk_aliased_vdefs (&refd, gimple_vuse (call), mark_modified,
> +		      &modified, NULL);
> +
> +  return modified;
> +}
> +
>  /* Go through arguments of the CALL and for every one that looks like a member
>     pointer, check whether it can be safely declared pass-through and if so,
>     mark that to the corresponding item of jump FUNCTIONS.  Return true iff
> @@ -733,7 +760,7 @@ compute_pass_through_member_ptrs (struct
>  	      int index = ipa_get_param_decl_index (info, arg);
>  
>  	      gcc_assert (index >=0);
> -	      if (!ipa_is_param_modified (info, index))
> +	      if (!is_parm_modified_at_call (call, arg))

Hm, I do not understand this.  Do you just want to ask whether
arg is modified _by_ the call?  Or do you want to know whether
arg is modified on any path leading from function entry to
the call (which is what you implement)?  In that case I suggest
a better name for the function, like is_parm_modified_before_call.

Note that you are not implementing any kind of limiting, doing many
of these walk can be very expensive.  For example in the above loop
I'd first check whether arg is a PARM_DECL before checking
type_like_member_ptr_p.  And you can use one caching bitmap
per PARM_DECL for the visited parms to walk_aliased_vdefs which
will bound complexity to number of cfun arguments times number
of stores in the function (much better than number of cfun arguments
times calls in the function times number of stores in the function).

Also why remove a testcase?  Please adjust it instead.

Please rework the patch according to the above comments.

Thanks,
Richard.

>  		{
>  		  functions[num].type = IPA_JF_PASS_THROUGH;
>  		  functions[num].value.pass_through.formal_id = index;
> @@ -1184,7 +1211,7 @@ ipa_analyze_indirect_call_uses (struct c
>      return;
>  
>    index = ipa_get_param_decl_index (info, rec);
> -  if (index >= 0 && !ipa_is_param_modified (info, index))
> +  if (index >= 0 && !is_parm_modified_at_call (call, rec))
>      ipa_note_param_call (node, index, call, false);
>  
>    return;
> @@ -1854,8 +1881,6 @@ ipa_print_node_params (FILE * f, struct
>                   (DECL_NAME (temp)
>                    ? (*lang_hooks.decl_printable_name) (temp, 2)
>                    : "(unnamed)"));
> -      if (ipa_is_param_modified (info, i))
> -	fprintf (f, " modified");
>        if (ipa_is_param_used (info, i))
>  	fprintf (f, " used");
>        fprintf (f, "\n");
> @@ -2470,10 +2495,7 @@ ipa_write_node_info (struct output_block
>    gcc_assert (!info->node_enqueued);
>    gcc_assert (!info->ipcp_orig_node);
>    for (j = 0; j < ipa_get_param_count (info); j++)
> -    {
> -      bp_pack_value (&bp, info->params[j].modified, 1);
> -      bp_pack_value (&bp, info->params[j].used, 1);
> -    }
> +    bp_pack_value (&bp, info->params[j].used, 1);
>    lto_output_bitpack (&bp);
>    for (e = node->callees; e; e = e->next_callee)
>      {
> @@ -2511,10 +2533,7 @@ ipa_read_node_info (struct lto_input_blo
>      }
>    info->node_enqueued = false;
>    for (k = 0; k < ipa_get_param_count (info); k++)
> -    {
> -      info->params[k].modified = bp_unpack_value (&bp, 1);
> -      info->params[k].used = bp_unpack_value (&bp, 1);
> -    }
> +    info->params[k].used = bp_unpack_value (&bp, 1);
>    for (e = node->callees; e; e = e->next_callee)
>      {
>        struct ipa_edge_args *args = IPA_EDGE_REF (e);
> Index: icln/gcc/ipa-prop.h
> ===================================================================
> --- icln.orig/gcc/ipa-prop.h	2010-06-20 17:56:03.000000000 +0200
> +++ icln/gcc/ipa-prop.h	2010-06-20 18:23:16.000000000 +0200
> @@ -161,8 +161,6 @@ struct ipa_param_descriptor
>    struct ipcp_lattice ipcp_lattice;
>    /* PARAM_DECL of this parameter.  */
>    tree decl;
> -  /* Whether the value parameter has been modified within the function.  */
> -  unsigned modified : 1;
>    /* The parameter is used.  */
>    unsigned used : 1;
>  };
> @@ -228,17 +226,6 @@ ipa_get_param (struct ipa_node_params *i
>    return info->params[i].decl;
>  }
>  
> -/* Return the modification flag corresponding to the Ith formal parameter of
> -   the function associated with INFO.  Note that there is no setter method as
> -   the goal is to set all flags when building the array in
> -   ipa_detect_param_modifications.  */
> -
> -static inline bool
> -ipa_is_param_modified (struct ipa_node_params *info, int i)
> -{
> -  return info->params[i].modified;
> -}
> -
>  /* Return the used flag corresponding to the Ith formal parameter of
>     the function associated with INFO.  */
>  
> Index: icln/gcc/testsuite/g++.dg/ipa/iinline-2.C
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ icln/gcc/testsuite/g++.dg/ipa/iinline-2.C	2010-06-20 18:23:16.000000000 +0200
> @@ -0,0 +1,64 @@
> +/* Verify that we do not indirect-inline using member pointer
> +   parameters which have been modified.  */
> +/* { dg-do run } */
> +/* { dg-options "-O3 -fno-early-inlining"  } */
> +/* { dg-add-options bind_pic_locally } */
> +
> +extern "C" void abort (void);
> +
> +class String
> +{
> +private:
> +  const char *data;
> +
> +public:
> +  String (const char *d) : data(d)
> +  {}
> +
> +  int funcOne (int stuff) const;
> +  int funcTwo (int stuff) const;
> +};
> +
> +
> +int String::funcOne (int stuff) const
> +{
> +  return stuff + 1;
> +}
> +
> +int String::funcTwo (int stuff) const
> +{
> +  return stuff + 100;
> +}
> +
> +int (String::* gmp)(int stuff) const = &String::funcTwo;
> +
> +int docalling_1 (int (String::* f)(int stuff) const)
> +{
> +  String S ("muhehehe");
> +
> +  return (S.*f)(4);
> +}
> +
> +int docalling (int a, int (String::* f)(int stuff) const)
> +{
> +  if (a < 200)
> +    f = gmp;
> +
> +  return docalling_1 (f);
> +}
> +
> +int __attribute__ ((noinline,noclone)) get_input (void)
> +{
> +  return 1;
> +}
> +
> +int main (int argc, char *argv[])
> +{
> +  int i = 0;
> +  while (i < 10)
> +    i += docalling (get_input (), &String::funcOne);
> +
> +  if (i != 104)
> +    abort();
> +  return 0;
> +}
> 
>

On Mon, 21 Jun 2010, Richard Guenther wrote:

> On Mon, 21 Jun 2010, Martin Jambor wrote:
> 
> > Hi,
> > 
> > indirect inlining was written before we had AA-info at our disposal in
> > the "early" passes and so when confronted with the necessity to
> > guarantee that a member pointer parameter was not changed before it
> > was used I have resorted to requiring it is never modified in the
> > whole function.
> > 
> > With alias analysis information this is no longer necessary.  This
> > code therefore does away with the flag and uses AA instead.  To big
> > extent I have copied the clobbering identification code from IPA-SRA
> > and hope I got it correct.
> > 
> > I added a new testcase checking that we so not indirect-inline when we
> > shouldn't because a member-pointer parameter is modified before being
> > passed on.
> > 
> > I am aware that there are no hunks removing testcase
> > gcc.dg/ipa/modif-1.c in the patch, that is because I generate my
> > patches with quilt rather than svn.  I will remember to svn-delete the
> > file.
> > 
> > I have bootstrapped and tested this patch (on top of the previous one)
> > on x86_64-linux without any issues,  OK for trunk?
> > 
> > Thanks,
> > 
> > Martin
> > 
> > 
> > 2010-06-18  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	* ipa-prop.h (struct ipa_param_descriptor): Remove the modified flag.
> > 	(ipa_is_param_modified): Removed.
> > 	* ipa-prop.c (visit_store_addr_for_mod_analysis): Do not set the
> > 	modified flag.
> > 	(mark_modified): New function.
> > 	(is_parm_modified_at_call): Likewise.
> > 	(compute_pass_through_member_ptrs): Use is_parm_modified_at_call
> > 	instead of ipa_is_param_modified.
> > 	(ipa_analyze_indirect_call_uses): Likewise.
> > 	(ipa_print_node_params): Do not dump the modified flag.
> > 	(ipa_write_node_info): Do not stream the modified flag.
> > 	(ipa_read_node_info): Likewise.
> > 
> > 	* testsuite/g++.dg/ipa/iinline-2.C: New test.
> > 	* testsuite/gcc.dg/ipa/modif-1.c: Removed.
> > 
> > Index: icln/gcc/ipa-prop.c
> > ===================================================================
> > --- icln.orig/gcc/ipa-prop.c	2010-06-20 18:23:13.000000000 +0200
> > +++ icln/gcc/ipa-prop.c	2010-06-20 18:24:32.000000000 +0200
> > @@ -212,7 +212,6 @@ visit_store_addr_for_mod_analysis (gimpl
> >      {
> >        int index = ipa_get_param_decl_index (info, op);
> >        gcc_assert (index >= 0);
> > -      info->params[index].modified = true;
> >        info->params[index].used = true;
> >      }
> >  
> > @@ -707,6 +706,34 @@ type_like_member_ptr_p (tree type, tree
> >    return true;
> >  }
> >  
> > +/* Callback of walk_aliased_vdefs.  Flags that it has been invoked to the
> > +   boolean variable pointed to by DATA.  */
> > +
> > +static bool
> > +mark_modified (ao_ref *ao ATTRIBUTE_UNUSED, tree vdef ATTRIBUTE_UNUSED,
> > +		     void *data)
> > +{
> > +  bool *b = (bool *) data;
> > +  *b = true;
> > +  return true;
> > +}
> > +
> > +/* Return true if the formal parameter PARM might have been modified in this
> > +   function before reaching the statement CALL.  */
> > +
> > +static bool
> > +is_parm_modified_at_call (gimple call, tree parm)
> > +{
> > +  bool modified = false;
> > +  ao_ref refd;
> > +
> > +  ao_ref_init (&refd, parm);
> > +  walk_aliased_vdefs (&refd, gimple_vuse (call), mark_modified,
> > +		      &modified, NULL);
> > +
> > +  return modified;
> > +}
> > +
> >  /* Go through arguments of the CALL and for every one that looks like a member
> >     pointer, check whether it can be safely declared pass-through and if so,
> >     mark that to the corresponding item of jump FUNCTIONS.  Return true iff
> > @@ -733,7 +760,7 @@ compute_pass_through_member_ptrs (struct
> >  	      int index = ipa_get_param_decl_index (info, arg);
> >  
> >  	      gcc_assert (index >=0);
> > -	      if (!ipa_is_param_modified (info, index))
> > +	      if (!is_parm_modified_at_call (call, arg))
> 
> Hm, I do not understand this.  Do you just want to ask whether
> arg is modified _by_ the call?  Or do you want to know whether
> arg is modified on any path leading from function entry to
> the call (which is what you implement)?  In that case I suggest
> a better name for the function, like is_parm_modified_before_call.
> 
> Note that you are not implementing any kind of limiting, doing many
> of these walk can be very expensive.  For example in the above loop
> I'd first check whether arg is a PARM_DECL before checking
> type_like_member_ptr_p.  And you can use one caching bitmap
> per PARM_DECL for the visited parms to walk_aliased_vdefs which
> will bound complexity to number of cfun arguments times number
> of stores in the function (much better than number of cfun arguments
> times calls in the function times number of stores in the function).

Thinking about this again you'd need to manage this caching
yourself somehow (and stop walking in the callback if you hit it).
You can keep a reaches-modification and doesn't-reach-modification
bitmap and OR them from the is_parm_modified_at_call local bitmap
depending on result.

Richard.

> Also why remove a testcase?  Please adjust it instead.
> 
> Please rework the patch according to the above comments.
> 
> Thanks,
> Richard.
> 
> >  		{
> >  		  functions[num].type = IPA_JF_PASS_THROUGH;
> >  		  functions[num].value.pass_through.formal_id = index;
> > @@ -1184,7 +1211,7 @@ ipa_analyze_indirect_call_uses (struct c
> >      return;
> >  
> >    index = ipa_get_param_decl_index (info, rec);
> > -  if (index >= 0 && !ipa_is_param_modified (info, index))
> > +  if (index >= 0 && !is_parm_modified_at_call (call, rec))
> >      ipa_note_param_call (node, index, call, false);
> >  
> >    return;
> > @@ -1854,8 +1881,6 @@ ipa_print_node_params (FILE * f, struct
> >                   (DECL_NAME (temp)
> >                    ? (*lang_hooks.decl_printable_name) (temp, 2)
> >                    : "(unnamed)"));
> > -      if (ipa_is_param_modified (info, i))
> > -	fprintf (f, " modified");
> >        if (ipa_is_param_used (info, i))
> >  	fprintf (f, " used");
> >        fprintf (f, "\n");
> > @@ -2470,10 +2495,7 @@ ipa_write_node_info (struct output_block
> >    gcc_assert (!info->node_enqueued);
> >    gcc_assert (!info->ipcp_orig_node);
> >    for (j = 0; j < ipa_get_param_count (info); j++)
> > -    {
> > -      bp_pack_value (&bp, info->params[j].modified, 1);
> > -      bp_pack_value (&bp, info->params[j].used, 1);
> > -    }
> > +    bp_pack_value (&bp, info->params[j].used, 1);
> >    lto_output_bitpack (&bp);
> >    for (e = node->callees; e; e = e->next_callee)
> >      {
> > @@ -2511,10 +2533,7 @@ ipa_read_node_info (struct lto_input_blo
> >      }
> >    info->node_enqueued = false;
> >    for (k = 0; k < ipa_get_param_count (info); k++)
> > -    {
> > -      info->params[k].modified = bp_unpack_value (&bp, 1);
> > -      info->params[k].used = bp_unpack_value (&bp, 1);
> > -    }
> > +    info->params[k].used = bp_unpack_value (&bp, 1);
> >    for (e = node->callees; e; e = e->next_callee)
> >      {
> >        struct ipa_edge_args *args = IPA_EDGE_REF (e);
> > Index: icln/gcc/ipa-prop.h
> > ===================================================================
> > --- icln.orig/gcc/ipa-prop.h	2010-06-20 17:56:03.000000000 +0200
> > +++ icln/gcc/ipa-prop.h	2010-06-20 18:23:16.000000000 +0200
> > @@ -161,8 +161,6 @@ struct ipa_param_descriptor
> >    struct ipcp_lattice ipcp_lattice;
> >    /* PARAM_DECL of this parameter.  */
> >    tree decl;
> > -  /* Whether the value parameter has been modified within the function.  */
> > -  unsigned modified : 1;
> >    /* The parameter is used.  */
> >    unsigned used : 1;
> >  };
> > @@ -228,17 +226,6 @@ ipa_get_param (struct ipa_node_params *i
> >    return info->params[i].decl;
> >  }
> >  
> > -/* Return the modification flag corresponding to the Ith formal parameter of
> > -   the function associated with INFO.  Note that there is no setter method as
> > -   the goal is to set all flags when building the array in
> > -   ipa_detect_param_modifications.  */
> > -
> > -static inline bool
> > -ipa_is_param_modified (struct ipa_node_params *info, int i)
> > -{
> > -  return info->params[i].modified;
> > -}
> > -
> >  /* Return the used flag corresponding to the Ith formal parameter of
> >     the function associated with INFO.  */
> >  
> > Index: icln/gcc/testsuite/g++.dg/ipa/iinline-2.C
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ icln/gcc/testsuite/g++.dg/ipa/iinline-2.C	2010-06-20 18:23:16.000000000 +0200
> > @@ -0,0 +1,64 @@
> > +/* Verify that we do not indirect-inline using member pointer
> > +   parameters which have been modified.  */
> > +/* { dg-do run } */
> > +/* { dg-options "-O3 -fno-early-inlining"  } */
> > +/* { dg-add-options bind_pic_locally } */
> > +
> > +extern "C" void abort (void);
> > +
> > +class String
> > +{
> > +private:
> > +  const char *data;
> > +
> > +public:
> > +  String (const char *d) : data(d)
> > +  {}
> > +
> > +  int funcOne (int stuff) const;
> > +  int funcTwo (int stuff) const;
> > +};
> > +
> > +
> > +int String::funcOne (int stuff) const
> > +{
> > +  return stuff + 1;
> > +}
> > +
> > +int String::funcTwo (int stuff) const
> > +{
> > +  return stuff + 100;
> > +}
> > +
> > +int (String::* gmp)(int stuff) const = &String::funcTwo;
> > +
> > +int docalling_1 (int (String::* f)(int stuff) const)
> > +{
> > +  String S ("muhehehe");
> > +
> > +  return (S.*f)(4);
> > +}
> > +
> > +int docalling (int a, int (String::* f)(int stuff) const)
> > +{
> > +  if (a < 200)
> > +    f = gmp;
> > +
> > +  return docalling_1 (f);
> > +}
> > +
> > +int __attribute__ ((noinline,noclone)) get_input (void)
> > +{
> > +  return 1;
> > +}
> > +
> > +int main (int argc, char *argv[])
> > +{
> > +  int i = 0;
> > +  while (i < 10)
> > +    i += docalling (get_input (), &String::funcOne);
> > +
> > +  if (i != 104)
> > +    abort();
> > +  return 0;
> > +}
> > 
> > 
> 
>