Re: [PATCH] True IPA reimplementation of IPA-SRA

[Martin Jambor <mjambor@suse.cz>]

Hi,

On Thu, Jun 13 2019, Jan Hubicka wrote:
> Hi,
> i read all changes except for ipa-sra itself.  Here are some comments,
> I will look at the remaining file next.
>
> Honza
>
>
> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> index 9a19d83fffb..3f838c08e76 100644
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
>   struct GTY(()) cgraph_clone_info
>   {
> +  /* Constants discovered by IPA-CP, i.e. which parameter should be 
> replaced
> +     with what.  */
>     vec<ipa_replace_map *, va_gc> *tree_map;
> -  bitmap args_to_skip;
> -  bitmap combined_args_to_skip;
> +  /* Parameter modification that IPA-SRA decided to perform.  */
> +  ipa_param_adjustments *param_adjustments;
> +  /* Lists of all splits with their offsets for each dummy variables
> +     representing a replaced-by-splits parameter.  */
> +  vec<ipa_param_performed_split, va_gc> *performed_splits;
>
> Please explain what dummy variables are :)

OK, I replaced the comment with:

  /* Lists of dummy-decl and offset pairs representing split formal parameters
     in the caller.  Offsets of all new replacements are enumerated, those
     coming from the same original parameter have the same dummy decl stored
     along with them.

     Dummy decls sit in call statement arguments followed by new parameter
     decls (or their SSA names) in between (caller) clone materialization and
     call redirection.  Redirection then recognizes the dummy variable and
     together with the stored offsets can reconstruct what exactly the new
     parameter decls represent and can leave in place only those that the
     callee expects.  */

>
> +/* Return true if we would like to remove a parameter from NODE when 
> cloning it
> +   with KNOWN_CSTS scalar constants.  */
> +
> +static bool
> +want_remove_some_param_p (cgraph_node *node, vec<tree> known_csts)
> +{
> +  auto_vec<bool, 16> surviving;
> +  bool filled_vec = false;
> +  ipa_node_params *info = IPA_NODE_REF (node);
> +  int i, count = ipa_get_param_count (info);
> +  for (i = 0; i < count; i++)
>
> vertical space after the declarations :)

OK

>
> -	  tree t = known_csts[i];
> -
> -	  if (t || !ipa_is_param_used (info, i))
> -	    bitmap_set_bit (args_to_skip, i);
> +	  ipa_adjusted_param *old_adj = &(*old_adjustments->m_adj_params)[i];
> +	  if (!node->local.can_change_signature
> +	      || old_adj->op != IPA_PARAM_OP_COPY
> +	      || (!known_csts[old_adj->base_index]
> +		  && ipa_is_param_used (info, old_adj->base_index)))
> +	    {
> +	      ipa_adjusted_param new_adj;
> +	      memcpy (&new_adj, old_adj, sizeof (new_adj));
>
> Why this is not *new_adj=*old_adj?

Not sure, I changed it to aggregate copy.

>
> +/* Names of parameters for dumping.  Keep in sync with enum 
> ipa_parm_op.  */
> +
> +static const char *ipa_param_op_names[] = {"IPA_PARAM_OP_UNDEFINED",
> +					   "IPA_PARAM_OP_COPY",
> +					   "IPA_PARAM_OP_NEW",
> +					   "IPA_PARAM_OP_SPLIT"};
>
> Given brave new C++ world, can't we statically assert that size of
> array match the enum?

Hm, I suppose so, I changed it.

> Also it seems to me that ipa-param-modification would benefit from
> some toplevel comment of what it does and what is the main API how
> to use it.
>

Right, so I wrote that, but it ended up a bit large, have a look at:

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/ipa-param-manipulation.h;h=b9da00bb5c9c813a0b16c7ec2f32e0d480f91238;hb=refs/heads/jamborm/ipa-sra


> Also functions like
>
> ipa_fill_vector_with_formal_parms
> ipa_fill_vector_with_formal_parm_types
>
> does not seem very IPA specific

They are basically replacements of currently existing functions:

  vec<tree> ipa_get_vector_of_formal_parms (tree fndecl);
  vec<tree> ipa_get_vector_of_formal_parm_types (tree fntype);

The difference is that the reimplementations do not directly return a
new heap allocated vector but fill a vector that the caller passes,
which makes it auto_vec friendly.

> and it was not quite obvoius from name what it does.  Perhaps it
> should go somewhere to common tree manipulatoin and have more
> fitting name?

I'm not sure what the problem with the name is, but...

> If it was something like push_function_arg_decls or
> push_function_arg_types it would be more obvious to me what it does
> :)

...if that is the case, OK, why not, I changed them.

As far as their placement is concerned, again, I don't care much.  I
can put them to tree.[hc] if you'd like (I haven't dome that that
yet).  It just so happens that people only use these when they mess
with parameters, hence they are in ipa-param-manipulation.[hc].

>
> Also I wonder if the code would not be more readable if functions was
> returning auto_vecs references. Then they can be just something like
> "function_arg_types" and the API would be self explanatory.

I probably don't understand.  References to auto_vecs that are in fact
stored where?

> +tree
> +ipa_param_adjustments::adjust_decl (tree orig_decl)
> +{
> +  tree new_decl = copy_node (orig_decl);
> +  tree orig_type = TREE_TYPE (orig_decl);
> +  if (prototype_p (orig_type)
> +      || (m_skip_return && !VOID_TYPE_P (TREE_TYPE (orig_type))))
> +    {
> +      tree new_type = build_new_function_type (orig_type, false);
> +      TREE_TYPE (new_decl) = new_type;
> +    }
> +  if (method2func_p (orig_type))
> +    DECL_VINDEX (new_decl) = NULL_TREE;
>
> I think you want to clear DECL_VINDEX in every case since the method
> is no longer one pointed to by virtual table. But I see we have
> similar code elsewhere so lets do that incrementally.
>

OK.  I have noted it down.

> With early debug info we probably can forget about it completely.

I can try that later too.

> +/* Structure to hold declarations representing transitive IPA-SRA 
> splits.  In
> +   essence, if we need to pass UNIT_OFFSET of a parameter which 
> originally has
> +   number BASE_INDEX, we should pass down REPL.  */
> +
> +struct transitive_split_map
> +{
> +  tree repl;
> +  unsigned base_index;
> +  unsigned unit_offset;
> +};
>
> It is not quite clear to me what those transitive splits are, so perhaps
> better description would help :)

I have dedicated a large section of the initial comment linked above
to the explanation of the term and how the patch handles such
situations.

> +  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> +  gimple_stmt_iterator prev_gsi = gsi;
> +  gsi_prev (&prev_gsi);
> I think we still maintain vertical space after declarations at the 
> beggining
> of block.  It may help to break out this function a bit. With all the 
> debug
> handling it is now quite monster.
> +/* Common initialization performed by all ipa_param_body_adjustments
> +   constructors.  OLD_FNDECL is the declaration we take original 
> arguments
> +   from, (it may be the same as M_FNDECL).  VARS, if non-NULL, is a 
> pointer to
> +   a chained list of new local variables.  TREE_MAP is the IPA-CP 
> produced
> +   mapping of trees to constants.  */
> +
> +void
> +ipa_param_body_adjustments::common_initialization (tree old_fndecl,
> +						   tree *vars,
> +						   vec<ipa_replace_map *,
> +						       va_gc> *tree_map)
>
> Some comments int he body about what the code does would help IMO :)
> It is bit of spagetti.

The function is rather long but it really only initializes all data
members of the class.  I tried to give the members clear names and
descriptions so that it would be clear what is what.  I tried to add
some comments nevertheless.

>
> diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h
> index 71fc4a201aa..85aa90e0403 100644
> --- a/gcc/ipa-param-manipulation.h
> +++ b/gcc/ipa-param-manipulation.h
> @@ -21,96 +21,282 @@ along with GCC; see the file COPYING3.  If not see
>   #ifndef IPA_PARAM_MANIPULATION_H
>   #define IPA_PARAM_MANIPULATION_H
>
> +/* Indices into ipa_param_prefixes to identify a human-readable prefix 
> for newly
> +   synthesized parameters.  Keep in sync with the array.  */
> +#define IPA_PARAM_PREFIX_SYNTH  0
> +#define IPA_PARAM_PREFIX_ISRA   1
> +#define IPA_PARAM_PREFIX_SIMD   2
> +#define IPA_PARAM_PREFIX_MASK   3
>
> Probably better as enum?

OK.

> diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
> index 5eaf8257f98..223dfeee7f9 100644
> --- a/gcc/ipa-split.c
> +++ b/gcc/ipa-split.c
>
> Eventually we want ot teach ipa-split to do more adjustments, like 
> adding new
> parameters for values computed in header that needs to be passed to tail 
> :)

That is definitely possible with the new infrastructure.
omp-simd-clone.c adds new parameters with it.  The price is that
enabling ipa_param_body_adjustments to work both as part the of call
graph cloning machinery and also in a standalone mode makes it a bit
bulky.


> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index 9bf1c4080f5..88895105bdc 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -191,7 +190,17 @@ remap_ssa_name (tree name, copy_body_data *id)
>
>     n = id->decl_map->get (name);
>     if (n)
> -    return unshare_expr (*n);
> +    {
> +      if (id->killed_new_ssa_names
> +	  && id->killed_new_ssa_names->contains (*n))
> +	{
> +	  gcc_assert (processing_debug_stmt);
> +	  processing_debug_stmt = -1;
> +	  return name;
> +	}
> +
> +      return unshare_expr (*n);
> +    }
>
>     if (processing_debug_stmt)
>       {
> @@ -1872,6 +1881,21 @@ remap_gimple_stmt (gimple *stmt, copy_body_data 
> *id)
>         gcc_assert (n);
>         gimple_set_block (copy, *n);
>       }
> +  if (id->param_body_adjs)
> +    {
> +      gimple_seq extra_stmts = NULL;
> +      id->param_body_adjs->modify_gimple_stmt (&copy, &extra_stmts);
> +      if (!gimple_seq_empty_p (extra_stmts))
> +	{
> +	  memset (&wi, 0, sizeof (wi));
> +	  wi.info = id;
> +	  for (gimple_stmt_iterator egsi = gsi_start (extra_stmts);
> +	       !gsi_end_p (egsi);
> +	       gsi_next (&egsi))
> +	    walk_gimple_op (gsi_stmt (egsi), remap_gimple_op_r, &wi);
> +	  gimple_seq_add_seq (&stmts, extra_stmts);
> +	}
> +    }

This hunk just calls into param_body_adjs to actually perform IPA-SRA
transformations on individual statements.  it has nothing to do with
killed_new_ssa_names.  Sometimes a transformed statement ends up as a
sequence of statements (simply because I need to create a conversion
statement if there is a type mismatch) and then I have to re-map all
of the resulting sequence and insert them into the new function.

> @@ -2794,10 +2818,20 @@ redirect_all_calls (copy_body_data * id, 
> basic_block bb)
>         gimple *stmt = gsi_stmt (si);
>         if (is_gimple_call (stmt))
>   	{
> +	  tree old_lhs = gimple_call_lhs (stmt);
>   	  struct cgraph_edge *edge = id->dst_node->get_edge (stmt);
>   	  if (edge)
>   	    {
>   	      edge->redirect_call_stmt_to_callee ();
> +	      if (old_lhs
> +		  && TREE_CODE (old_lhs) == SSA_NAME
> +		  && !gimple_call_lhs (edge->call_stmt))
> +		{
> +		  if (!id->killed_new_ssa_names)
> +		    id->killed_new_ssa_names = new hash_set<tree> (16);
> +		  id->killed_new_ssa_names->add (old_lhs);
> +		}
> +
>
> Some enlightening comments would help for those three hunks :)
> So if LHS of call died you need to get rid of it.
> I wonder if one can't also just store 0 to it and let later cleanups to
> do their job?

The dead SSAs only appeared in debug statements, they belong to return
values which are otherwise unused.  I'm afraid we'd then generate
debug info claiming the value is zero (but I admit I have not tried).
But I added comments in the two hunks manipulating
killed_new_ssa_names.


> @@ -5653,7 +5721,7 @@ copy_decl_for_dup_finish (copy_body_data *id, tree 
> decl, tree copy)
>     return copy;
>   }
>
> -static tree
> +tree
>   copy_decl_to_var (tree decl, copy_body_data *id)
>   {
>     tree copy, type;
>
> Add a comment what function does, especially when it is exported now ;)

OK, thanks, I'm looking forward to any further suggestions.

Martin