Re: [patch] handle new cgraph nodes in ipa-cp pass

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Thu, Apr 01, 2010 at 06:23:54PM -0400, Aldy Hernandez wrote:
> Hi.
> 
> I'm fixing some problems exhibited by TM benchmarks on the
> transactional-memory branch.
> 
> In this particular problem, ipa_lattice_meet() dies because we have no
> associated parameter/argument info corresponding to a given callee
> (IPA_NODE_REF).  The callee in question is a transactional clone, which
> got created by the IPA TM pass (pass_ipa_tm).

I don't see how ipa_lattice_meet (in trunk) could fail in such a way.
I assume you mean one of the IPA_NODE_REFs in its caller
ipcp_propagate_stage (and by dying you mean a vector index error).
That is indeed very strange because the call to
ipa_check_create_node_params at the beginning of the function is
there to avoid exactly these kinds of problems.  How do you create
the callee?  Specifically, is cgraph_max_uid incremented accordingly? 

> 
> The problem is that the summary passes for the all_regular_ipa_passes
> (which both pass_ipa_cp and pass_ipa_tm belong to) gets run before we
> run the actual TM pass.  Since the ipa-cp summary pass which initializes
> IPA_NODE_REF gets called before the TM clones get created in the TM
> pass, we end up with uninitialized node information.
> 
> The ipa-cp pass is actually broken in that it should define 
> cgraph_add_function_insertion_hook() to handle new cgraph nodes that get
> created after the summary pass runs, but before the actual pass runs.
> This seems like a latent bug that is bound to cause problems if any
> member of all_regular_ipa_passes that runs before ipa-cp creates a new
> cgraph node (ok, so it's only pass_ipa_whole_program_visibility
> now).

Unfortunately, this idea will not work in WHOPR because in WHOPR the
bodies of the function are not accessible and so IPA-CP cannot analyze
it.  This is by the way the main reason why the local and global
analysis is broken up this way.  So having a callback for new nodes
that will analyze it as usual will not work in principle (or at least
not unless we undergo some paradigm shift that would however
unavoidably hurt scalability).

I believe that the problem can be avoided by making
ipa_check_create_node_params do its job properly and having ipa-cp
ignore the new nodes then.  We may find out that ipa-cp needs a new
flag in struct ipa_node_params that would say "ipa-cp analysis or node
duplication hook has seen this node" and ignore all other nodes much
in the same way it ignores those with called_with_var_arguments set.
Of course, ideally the pass that created the node would make up for
the missed analysis and filled in info with correct values but this
may cease to be practical with the increasing number of IPA passes.

I'll be glad to help with tackling this as outlined above,

Martin


> 
> The patch below adds the appropriate hook, and fixes the pass ordering
> problem I'm seeing in trans-mem land.
> 
> Is this OK for trunk right before the cut-off, or perhaps to be queued
> for 4.6?  (I'd hate to put it only on the [trans-mem] branch, since it
> can affect non TM code, but I could put it there if I'm forced to :)).
> 
> Thanks.
> 
> 	* ipa-cp.c (function_insertion_hook_holder): New global.
> 	(add_new_function): New.
> 	(ipcp_driver): Remove cgraph insertion hook.
> 	(ipcp_generate_summary): Add cgraph insertion hook.
> 	(ipcp_read_summary): Same.
> 
> Index: ipa-cp.c
> ===================================================================
> --- ipa-cp.c	(revision 157917)
> +++ ipa-cp.c	(working copy)
> @@ -150,6 +150,9 @@ static bitmap dead_nodes;
>  static void ipcp_print_profile_data (FILE *);
>  static void ipcp_function_scale_print (FILE *);
>  
> +/* Holders of ipa cgraph hooks: */
> +static struct cgraph_node_hook_list *function_insertion_hook_holder;
> +
>  /* Get the original node field of ipa_node_params associated with node NODE.  */
>  static inline struct cgraph_node *
>  ipcp_get_orig_node (struct cgraph_node *node)
> @@ -186,6 +189,13 @@ ipcp_analyze_node (struct cgraph_node *n
>    ipa_detect_param_modifications (node);
>  }
>  
> +/* Called when new function is inserted to callgraph late.  */
> +static void
> +add_new_function (struct cgraph_node *node, void *data ATTRIBUTE_UNUSED)
> +{
> +  ipcp_analyze_node (node);
> +}
> +
>  /* Return scale for NODE.  */
>  static inline gcov_type
>  ipcp_get_node_scale (struct cgraph_node *node)
> @@ -1251,7 +1261,8 @@ ipcp_insert_stage (void)
>  static unsigned int
>  ipcp_driver (void)
>  {
> -  cgraph_remove_unreachable_nodes (true,dump_file);
> +  cgraph_remove_function_insertion_hook (function_insertion_hook_holder);
> +  cgraph_remove_unreachable_nodes (true, dump_file);
>    if (dump_file)
>      {
>        fprintf (dump_file, "\nIPA structures before propagation:\n");
> @@ -1281,6 +1292,8 @@ ipcp_generate_summary (void)
>  {
>    if (dump_file)
>      fprintf (dump_file, "\nIPA constant propagation start:\n");
> +  function_insertion_hook_holder =
> +    cgraph_add_function_insertion_hook (&add_new_function, NULL);
>    ipa_check_create_node_params ();
>    ipa_check_create_edge_args ();
>    ipa_register_cgraph_hooks ();
> @@ -1301,6 +1314,8 @@ static void
>  ipcp_read_summary (void)
>  {
>    ipa_prop_read_jump_functions ();
> +  function_insertion_hook_holder =
> +    cgraph_add_function_insertion_hook (&add_new_function, NULL);
>  }
>  
>  /* Gate for IPCP optimization.  */