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

[Jan Hubicka <jh@suse.de>]

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 :)

+/* 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 :)

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

+/* 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?
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.

Also functions like

ipa_fill_vector_with_formal_parms
ipa_fill_vector_with_formal_parm_types

does not seem very IPA specific 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?

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

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

With early debug info we probably can forget about it completely.
+/* 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 :)
+  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.

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?
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 
:)
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);
+	}
+    }
@@ -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?
@@ -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 ;)