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

Hi,

On Mon, Jun 21, 2010 at 02:21:41PM +0200, Richard Guenther wrote:
> 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.

OK, I have renamed the function, among other things.

> > 
> > Note that you are not implementing any kind of limiting, doing many
> > of these walk can be very expensive.

Right, I wanted to start with something simple but limiting should be
done.

> >  For example in the above loop
> > I'd first check whether arg is a PARM_DECL before checking
> > type_like_member_ptr_p. 

In compute_pass_through_member_ptrs, the result of
type_like_member_ptr_p is then used to determine whether we will scan
for constant member pointer arguments, hoping that most of the time we
won't.

> > 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.
> 

Well, I thought about this and eventually came to the conclusion that
trying to do member pointer indirect inlining (and these AA queries
are not used for anything else) in presence of complex modifications
of the parameters probably isn't worth the complexity.  Therefore I
decided to use a single visited_stmts bitmap and if
is_parm_modified_before_call returns true once, make it return true
for that parameter always.

This might not bring much more accuracy than we have now but has the
benefit of being able to avoid one complete walk of the function.
Currently, we need to get the modified flag correct by walking the
whole function before we embark on examining call statements looking
for indirect calls (including those of member pointers) and
calculating jump functions.  If we replace the modified flag with AA,
this is not necessary and the whole analysis can be done in one walk.

Moreover, we currently do two sweeps over all nodes just to get all
parameter descriptors and parameter counts initialized and to
calculate the modified flag for all nodes before computing the jump
functions.  If we do the initialization lazily, that is not necessary
either.

And that is what the patch below does.  It still includes a removal of
gcc.dg/ipa/modif-1.c because that is just a dump scan for the result
of modification analysis which we do not perform with this patch.  I
did add a new testcase that actually tests we get AA-queries right so
the coverage should not get worse.

I have bootstrapped and tested a very similar variant on x86-64-linux
without any problems, a bootstrap and test of exactly this version is
underway.  What so you think now?

Thanks,

Martin

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

	* ipa-prop.h (struct ipa_param_descriptor): Removed the modified flag.
	(struct ipa_node_params): Removed the modification_analysis_done flag.
	(ipa_is_param_modified): Removed.
	(ipa_analyze_node): Declare.
	(ipa_compute_jump_functions): Remove declaration.
	(ipa_count_arguments): Likewise.
	(ipa_detect_param_modifications): Likewise.
	(ipa_analyze_params_uses): Likewise.
	* ipa-prop.c (struct param_analysis_info): New type.
	(visit_store_addr_for_mod_analysis): Removed.
	(visit_load_for_mod_analysis): Renamed to visit_ref_for_mod_analysis,
	moved down in the file.
	(ipa_detect_param_modifications): Merged into ipa_analyze_params_uses.
	(ipa_count_arguments): Made static.
	(mark_modified): New function.
	(is_parm_modified_before_call): New function.
	(compute_pass_through_member_ptrs): New parameter parms_info, call
	is_parm_modified_before_call instead of ipa_is_param_modified.
	(ipa_compute_jump_functions_for_edge): New parameter parms_info, pass
	it to compute_pass_through_member_ptrs.
	(ipa_compute_jump_functions): New parameter parms_info, pass it to
	ipa_compute_jump_functions_for_edge.  Call ipa_initialize_node_params
	on the callee if it is analyzed.  Made static.
	(ipa_analyze_indirect_call_uses): New parameter parms_info, call
	is_parm_modified_before_call instead of ipa_is_param_modified.
	(ipa_analyze_call_uses): New parameter parms_info, pass it to
	ipa_analyze_indirect_call_uses.
	(ipa_analyze_stmt_uses): New parameter parms_info, pass it to
	ipa_analyze_call_uses.
	(ipa_analyze_params_uses): New parameter parms_info, pass it to
	ipa_analyze_stmt_uses.  Also perform the used analysis.  Made static.
	(ipa_analyze_node): New function.
	(ipa_print_node_params): Do not dump the modified flag.
	(ipa_write_node_info): Assert uses_analysis_done rather than streaming
	it.  Do not stream the modified parameter flag.
	(ipa_read_node_info): Set uses_analysis_done to 1 instead of streaming
	it.  Do not stream the modified parameter flag.
	* ipa-cp.c (ipcp_analyze_node): Removed.
	(ipcp_init_stage): Iterate only once over the nodes, analyze each one
	with only a call to ipa_analyze_node.
	* ipa-inline.c (inline_indirect_intraprocedural_analysis): Analyze the
	node with only a call to ipa_analyze_node.

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

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

Commit Message

Comments

Patch