Re: [tree-profiling-branch PATCH] Function cloning + IPCP extension (RESUBMISSION)

[Jan Hubicka <jh at suse dot cz>]

> Hello,
> 
> Attached is the patch modified according to the suggestions
> we received.
> 
> This patch implements function cloning.
> It also contains extension to IPCP (Interprocedural constant propagation) 
> optimization http://gcc.gnu.org/ml/gcc-patches/2004-08/msg00998.html for 
> using function cloning.
> 
> Now in order to enable IPCP for a given application, we should use the 
> command
> gcc -fipa-cp -combine file1.c file2.c file3.c 
> where file1.c, file2.c, file3.c are the files of the application.
> 
> Bootstrapped and regression tests successfully passed on POWER4.
> 
> Comments are welcome.

Hi,
The cgraph bits looks almost fine to me now (modulo minor problems),
but I still have few questions ;)

Index: cgraph.c
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/cgraph.c,v
retrieving revision 1.4.4.18.2.8
diff -c -3 -p -r1.4.4.18.2.8 cgraph.c
*** cgraph.c	25 Sep 2004 23:17:32 -0000	1.4.4.18.2.8
--- cgraph.c	5 Oct 2004 08:35:54 -0000
*************** cgraph_unnest_node (struct cgraph_node *
*** 706,709 ****
--- 706,737 ----
    *node2 = node->next_nested;
    node->origin = NULL;
  }
+ 
+ /* Copy collees of node FROM to node TO.  */
+ void
+ cgraph_copy_node_callees (struct cgraph_node *to,
+                           struct cgraph_node *from)
+ {
+   struct cgraph_edge *e;
+   
+   for (e = from->callees;e; e=e->next_callee)
+     cgraph_clone_edge (e, to, e->call_expr);
+   
+ }
+ 
+ 
+ void
+ cgraph_copy_local_info (struct cgraph_node *new_node, 
+ 			struct cgraph_node *orig_node)
+ {
+   new_node->local.self_insns = orig_node->local.self_insns;
+   new_node->local.local = orig_node->local.local;
+   new_node->local.finalized = orig_node->local.finalized;
+   new_node->local.disregard_inline_limits = 
+     orig_node->local.disregard_inline_limits;
+   new_node->local.inlinable = orig_node->local.inlinable;
+   new_node->local.redefined_extern_inline = 
+     orig_node->local.redefined_extern_inline;
+ }

The function lacks comment.
Why new_node->local = old_node->local won't work?  This is how the info
is copied in the cgraph_clone_node.

If you want to have special function for this, it is OK with me, but
please make it used by the clone_node function (same for
cgraph_copy_node_callees)

+ /* Get first callee of node.  */
+ #define FIRST_CALLEE(node)               ((node)->callees)
+ /* Get next callee from edge.  */
+ #define NEXT_CALLEE(edge)                ((edge)->next_callee)
+ /* Returns whether edge is last callee.  */
+ #define CALLEE_NOT_LAST(edge)            ((edge) != NULL)
+ 

Similarly if you want to have the macros, please make them used
consistently.  I tend to preffer node->callees instead of FIRST_CALLEE
tought, but since we have both schemes in GCC already, I am fine.
The CALLEE_NOT_LAST should be probably CALLEE_NOT_LAST_P as it is
predicate and I would preffer it to be CALLEE_LAST_P to avoid double
negatives.

+ struct cgraph_node *
+ cgraph_copy_node_for_versioning (struct cgraph_node *old_version,
+  				 tree new_decl, varray_type redirect_callers)
+ {
+   struct cgraph_node *new_version;
+   struct cgraph_edge *e;
+   struct cgraph_edge *next_callee;
+   unsigned i;
+   
+   if (old_version == NULL)
+     abort ();
+   
+   new_version = cgraph_node (new_decl);
+   
+   new_version->needed = false; 
+   new_version->decl = new_decl;

cgraph_node should make this job for you.  Perhaps adding "new_decl"
argument to cgraph_clone_node would kill most of the redundancy with
this function?  (Ie you will need to call cgraph_clone_node and 
then fixup the recursive edges).  When you do have recursion, why you
want the recursive call to always point to new clone (I would guess that
the function can actually change the value of operand)..
Index: cp/tree.c
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/cp/tree.c,v
retrieving revision 1.286.2.43.2.11
diff -c -3 -p -r1.286.2.43.2.11 tree.c
*** cp/tree.c	25 Sep 2004 23:18:41 -0000	1.286.2.43.2.11
--- cp/tree.c	5 Oct 2004 08:35:58 -0000
*************** cp_cannot_inline_tree_fn (tree* fnp)
*** 2057,2062 ****
--- 2057,2088 ----
    return 0;
  }
  
+ /* Decide whether there are language-specific reasons to not version a
+    function as a tree.  */
+ 
+ int 
+ cp_cannot_version_tree_fn (tree *fnp)
+ {
+   tree fn = *fnp;
+  
+   /* ??? Is there a way to version a
+      constructor?  */ 
+   if (DECL_COMPLETE_CONSTRUCTOR_P (fn))
+     return 1;
+   if (DECL_BASE_CONSTRUCTOR_P (fn))
+     return 1;
+   if (DECL_COMPLETE_DESTRUCTOR_P (fn))
+     return 1;
+   if (DECL_BASE_DESTRUCTOR_P (fn))
+     return 1;
+   if (DECL_OVERLOADED_OPERATOR_P (fn))
+     return 1;

Why this is needed?
(ie when you have already lowered form of the program you should either
see the low level information why clonning is not possible or you should
be able to clone it).

Otherwise the patch looks almost OK to me (with the incremental plan to
work on CFG based inlining/cloning next :)
Please also incorporate the Steven's comments (especially one about
moving the function out of integrate.c - that file should die soon). I
think it is resonable to place new functions in cgraph/cgraphunit.c as
they are pretty trivial now and the main infrastructure should have
friendly way to clone the function)

Also, please, prepare small testsuite catching the cases you can deal
with now.  Since we lack the IPA directory in testsuite, it would be
nice to add one. See tree-ssa directory on how to parse the dump files,
in fact copying around it's .exp file should (I hope) mostly work for
us.

Like Steven, I would be also interested to hear about the results in
some benchmarks (at least absolute numbers about how much clonning you
do and how much arguemnts you find to be constant).

Thank you,
Honza