Patchwork [PR,46984] Move thunk_delta from cgraph_indirect_call_info to cgraph_edge

login
register
mail settings
Submitter Martin Jambor
Date Dec. 21, 2010, 2:51 p.m.
Message ID <20101221145143.GA1298@virgil.arch.suse.de>
Download mbox | patch
Permalink /patch/76297/
State New
Headers show

Comments

Martin Jambor - Dec. 21, 2010, 2:51 p.m.
Hi,

I made a stupid mistake when adding streaming of the new thunk_delta
field in cgraph_indirect_call_info.  Without much thinking I simply
added it to the existing streaming but forgot that the streaming deals
with the analysis information, not decision information (for which
ipa-prop has no streaming).  This is problem because thunk_delta is
not an analysis thing but is determined and stored when making
inlining and ipa-cp decisions and must be made available to the
transformation phase.

I think that adding decision streaming to ipa-prop just for this
purpose is not really a good idea (ipa-prop decisions are basically
stored in call graph modifications and streamed along with call graph)
and so I moved the field to the cgraph_edge and made it a
HOST_WIDE_INT since we cannot stream trees along with the call graph.

I have bootstrapped and tested this on x86_64-linux without any
problems.  OK for trunk?

Thanks,

Martin


2010-12-20  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/46984
	* cgraph.h (cgraph_indirect_call_info): Remove field thunk_delta.
	(cgraph_edge): New field thunk_edlta.
	(cgraph_make_edge_direct) Update declaration.
	* cgraph.c (cgraph_create_edge_1): Initialize thunk_delta field.
	(cgraph_make_edge_direct): Made delta HOST_WIDE_INT, copy it to the
	new cgraph field.  Updated all callees.
	(cgraph_clone_edge): Copy thunk_delta.
	* cgraphunit.c (cgraph_redirect_edge_call_stmt_to_callee): Update for
	the new thunk_delta representation.
	* ipa-prop.c (ipa_make_edge_direct_to_target): Convert delta to
	HOST_WIDE_INT.
	(ipa_write_indirect_edge_info): Remove streaming of thunk_delta.
	(ipa_read_indirect_edge_info): Likewise.
	* lto-cgraph.c (lto_output_edge): Stream thunk_delta.
	(input_edge): Likewise.

	* testsuite/g++.dg/ipa/pr46984.C: New test.
Jan Hubicka - Dec. 23, 2010, 11:49 a.m.
V/lt> Hi,
> 
> I made a stupid mistake when adding streaming of the new thunk_delta
> field in cgraph_indirect_call_info.  Without much thinking I simply
> added it to the existing streaming but forgot that the streaming deals
> with the analysis information, not decision information (for which
> ipa-prop has no streaming).  This is problem because thunk_delta is
> not an analysis thing but is determined and stored when making
> inlining and ipa-cp decisions and must be made available to the
> transformation phase.
> 
> I think that adding decision streaming to ipa-prop just for this
> purpose is not really a good idea (ipa-prop decisions are basically
> stored in call graph modifications and streamed along with call graph)
> and so I moved the field to the cgraph_edge and made it a
> HOST_WIDE_INT since we cannot stream trees along with the call graph.

> 
> I have bootstrapped and tested this on x86_64-linux without any
> problems.  OK for trunk?
> Index: icln/gcc/lto-cgraph.c
> ===================================================================
> --- icln.orig/gcc/lto-cgraph.c
> +++ icln/gcc/lto-cgraph.c
> @@ -278,6 +278,7 @@ lto_output_edge (struct lto_simple_outpu
>      }
>  
>    lto_output_sleb128_stream (ob->main_stream, edge->count);
> +  lto_output_sleb128_stream (ob->main_stream, edge->thunk_delta);
>  
>    bp = bitpack_create (ob->main_stream);
>    uid = (!gimple_has_body_p (edge->caller->decl)
> @@ -1210,6 +1211,7 @@ input_edge (struct lto_input_block *ib,
>    struct cgraph_edge *edge;
>    unsigned int stmt_id;
>    gcov_type count;
> +  HOST_WIDE_INT thunk_delta;
>    int freq;
>    unsigned int nest;
>    cgraph_inline_failed_t inline_failed;
> @@ -1230,6 +1232,7 @@ input_edge (struct lto_input_block *ib,
>      callee = NULL;
>  
>    count = (gcov_type) lto_input_sleb128 (ib);
> +  thunk_delta = lto_input_sleb128 (ib);
>  
>    bp = lto_input_bitpack (ib);
>    stmt_id = (unsigned int) bp_unpack_value (&bp, HOST_BITS_PER_INT);
> @@ -1243,6 +1246,7 @@ input_edge (struct lto_input_block *ib,
>    else
>      edge = cgraph_create_edge (caller, callee, NULL, count, freq, nest);
>  
> +  edge->thunk_delta = thunk_delta;
>    edge->indirect_inlining_edge = bp_unpack_value (&bp, 1);
>    edge->lto_stmt_uid = stmt_id;
>    edge->inline_failed = inline_failed;

You want to store this kind of info into cgraphopt section. (probably adding
output_egge_opt_summary function).
Consider patch pre-approved with that change.

Honza
Martin Jambor - Dec. 26, 2010, 8:54 p.m.
Hi,

On Thu, Dec 23, 2010 at 12:49:16PM +0100, Jan Hubicka wrote:
> V/lt> Hi,
> > 
> > I made a stupid mistake when adding streaming of the new thunk_delta
> > field in cgraph_indirect_call_info.  Without much thinking I simply
> > added it to the existing streaming but forgot that the streaming deals
> > with the analysis information, not decision information (for which
> > ipa-prop has no streaming).  This is problem because thunk_delta is
> > not an analysis thing but is determined and stored when making
> > inlining and ipa-cp decisions and must be made available to the
> > transformation phase.
> > 
> > I think that adding decision streaming to ipa-prop just for this
> > purpose is not really a good idea (ipa-prop decisions are basically
> > stored in call graph modifications and streamed along with call graph)
> > and so I moved the field to the cgraph_edge and made it a
> > HOST_WIDE_INT since we cannot stream trees along with the call graph.
> 
> > 
> > I have bootstrapped and tested this on x86_64-linux without any
> > problems.  OK for trunk?
> > Index: icln/gcc/lto-cgraph.c
> > ===================================================================
> > --- icln.orig/gcc/lto-cgraph.c
> > +++ icln/gcc/lto-cgraph.c
> > @@ -278,6 +278,7 @@ lto_output_edge (struct lto_simple_outpu
> >      }
> >  
> >    lto_output_sleb128_stream (ob->main_stream, edge->count);
> > +  lto_output_sleb128_stream (ob->main_stream, edge->thunk_delta);
> >  
> >    bp = bitpack_create (ob->main_stream);
> >    uid = (!gimple_has_body_p (edge->caller->decl)
> > @@ -1210,6 +1211,7 @@ input_edge (struct lto_input_block *ib,
> >    struct cgraph_edge *edge;
> >    unsigned int stmt_id;
> >    gcov_type count;
> > +  HOST_WIDE_INT thunk_delta;
> >    int freq;
> >    unsigned int nest;
> >    cgraph_inline_failed_t inline_failed;
> > @@ -1230,6 +1232,7 @@ input_edge (struct lto_input_block *ib,
> >      callee = NULL;
> >  
> >    count = (gcov_type) lto_input_sleb128 (ib);
> > +  thunk_delta = lto_input_sleb128 (ib);
> >  
> >    bp = lto_input_bitpack (ib);
> >    stmt_id = (unsigned int) bp_unpack_value (&bp, HOST_BITS_PER_INT);
> > @@ -1243,6 +1246,7 @@ input_edge (struct lto_input_block *ib,
> >    else
> >      edge = cgraph_create_edge (caller, callee, NULL, count, freq, nest);
> >  
> > +  edge->thunk_delta = thunk_delta;
> >    edge->indirect_inlining_edge = bp_unpack_value (&bp, 1);
> >    edge->lto_stmt_uid = stmt_id;
> >    edge->inline_failed = inline_failed;
> 
> You want to store this kind of info into cgraphopt section. (probably adding
> output_egge_opt_summary function).
> Consider patch pre-approved with that change.
> 

I guess that I'd have to add a vector of edges to lto_cgraph_encoder_d
and to input_cgraph, input_cgraph_opt_summary and its callees.  Hardly
something to be pre-approved :-)

I've already written some of this but before I finish it, test it and
re-post, I'd like to ask you to re-consider.  If we ever build call
graph edges for calls to thunks (especially as we plan to be able to
do it when building the graph), such thunk information would become an
integral part of an edge, without which the edge has an invalid
meaning.  I'd therefore not consider this an "optimization"
information in the sense that it is somehow optional (unlike
args_to_skip or tree_map which we can ignore without miscompiling).

Or do I somehow misunderstand the intended meaning of the cgraphopt
section?

Thanks,

Martin
Jan Hubicka - Dec. 27, 2010, 12:39 a.m.
> I guess that I'd have to add a vector of edges to lto_cgraph_encoder_d
> and to input_cgraph, input_cgraph_opt_summary and its callees.  Hardly
> something to be pre-approved :-)
> 
> I've already written some of this but before I finish it, test it and
> re-post, I'd like to ask you to re-consider.  If we ever build call
> graph edges for calls to thunks (especially as we plan to be able to
> do it when building the graph), such thunk information would become an
> integral part of an edge, without which the edge has an invalid
> meaning.  I'd therefore not consider this an "optimization"
> information in the sense that it is somehow optional (unlike
> args_to_skip or tree_map which we can ignore without miscompiling).
> 
> Or do I somehow misunderstand the intended meaning of the cgraphopt
> section?

Well, cgraphopt stands for cgraph optimization summary that is everything
streamed down from wpa to ltrans but not from lgen to wpa.
I see that we might want to store thunk info on edges to represent the
direct calls to thunks, but this opens another cam of worms since what
is thunk in one unit don't need to be tunk in another. So I think we rather
want to lower the calls...

Honza
> 
> Thanks,
> 
> Martin
Martin Jambor - Dec. 27, 2010, 10:24 a.m.
On Mon, Dec 27, 2010 at 01:39:43AM +0100, Jan Hubicka wrote:
> > I guess that I'd have to add a vector of edges to lto_cgraph_encoder_d
> > and to input_cgraph, input_cgraph_opt_summary and its callees.  Hardly
> > something to be pre-approved :-)
> > 
> > I've already written some of this but before I finish it, test it and
> > re-post, I'd like to ask you to re-consider.  If we ever build call
> > graph edges for calls to thunks (especially as we plan to be able to
> > do it when building the graph), such thunk information would become an
> > integral part of an edge, without which the edge has an invalid
> > meaning.  I'd therefore not consider this an "optimization"
> > information in the sense that it is somehow optional (unlike
> > args_to_skip or tree_map which we can ignore without miscompiling).
> > 
> > Or do I somehow misunderstand the intended meaning of the cgraphopt
> > section?
> 
> Well, cgraphopt stands for cgraph optimization summary that is everything
> streamed down from wpa to ltrans but not from lgen to wpa.
> I see that we might want to store thunk info on edges to represent the
> direct calls to thunks, but this opens another cam of worms since what
> is thunk in one unit don't need to be tunk in another. So I think we rather
> want to lower the calls...

Fair enough.  However, if we try to lower the calls to thunks up
front, we might actually want to keep the thunk_delta field in
cgraph_indirect_call_info in order to save memory for the vast
majority of call graph edges that have nothing to do with thunks (but
still convert it to HOST_WIDE_INT and stream as such in cgraphopt).

I'll continue working on this along these lines then.  Thanks,

Martin

Patch

Index: icln/gcc/cgraph.c
===================================================================
--- icln.orig/gcc/cgraph.c
+++ icln/gcc/cgraph.c
@@ -860,7 +860,7 @@  cgraph_set_call_stmt (struct cgraph_edge
 	 indirect call into a direct one.  */
       struct cgraph_node *new_callee = cgraph_node (decl);
 
-      cgraph_make_edge_direct (e, new_callee, NULL);
+      cgraph_make_edge_direct (e, new_callee, 0);
     }
 
   push_cfun (DECL_STRUCT_FUNCTION (e->caller->decl));
@@ -1022,6 +1022,7 @@  cgraph_create_edge_1 (struct cgraph_node
   edge->next_caller = NULL;
   edge->prev_callee = NULL;
   edge->next_callee = NULL;
+  edge->thunk_delta = 0;
 
   edge->count = count;
   gcc_assert (count >= 0);
@@ -1195,15 +1196,15 @@  cgraph_redirect_edge_callee (struct cgra
 }
 
 /* Make an indirect EDGE with an unknown callee an ordinary edge leading to
-   CALLEE.  DELTA, if non-NULL, is an integer constant that is to be added to
-   the this pointer (first parameter).  */
+   CALLEE.  DELTA is an integer constant that is to be added to the this
+   pointer (first parameter) to compensate for skipping a thunk adjustment.  */
 
 void
 cgraph_make_edge_direct (struct cgraph_edge *edge, struct cgraph_node *callee,
-			 tree delta)
+			 HOST_WIDE_INT delta)
 {
   edge->indirect_unknown_callee = 0;
-  edge->indirect_info->thunk_delta = delta;
+  edge->thunk_delta = delta;
 
   /* Get the edge out of the indirect edge list. */
   if (edge->prev_callee)
@@ -2130,6 +2131,7 @@  cgraph_clone_edge (struct cgraph_edge *e
 	}
     }
 
+  new_edge->thunk_delta = e->thunk_delta;
   new_edge->inline_failed = e->inline_failed;
   new_edge->indirect_inlining_edge = e->indirect_inlining_edge;
   new_edge->lto_stmt_uid = stmt_uid;
Index: icln/gcc/cgraph.h
===================================================================
--- icln.orig/gcc/cgraph.h
+++ icln/gcc/cgraph.h
@@ -388,9 +388,6 @@  struct GTY(()) cgraph_indirect_call_info
   HOST_WIDE_INT otr_token;
   /* Type of the object from OBJ_TYPE_REF_OBJECT. */
   tree otr_type;
-  /* Delta by which must be added to this parameter.  For polymorphic calls
-     only.  */
-  tree thunk_delta;
   /* Index of the parameter that is called.  */
   int param_index;
   /* ECF flags determined from the caller.  */
@@ -404,6 +401,10 @@  struct GTY(()) cgraph_indirect_call_info
 struct GTY((chain_next ("%h.next_caller"), chain_prev ("%h.prev_caller"))) cgraph_edge {
   /* Expected number of executions: calculated in profile.c.  */
   gcov_type count;
+  /* Delta by which must be added to this parameter to compensate for a skipped
+     this adjusting thunk.  */
+  HOST_WIDE_INT thunk_delta;
+
   struct cgraph_node *caller;
   struct cgraph_node *callee;
   struct cgraph_edge *prev_caller;
@@ -578,7 +579,8 @@  struct cgraph_node * cgraph_clone_node (
 					int, bool, VEC(cgraph_edge_p,heap) *);
 
 void cgraph_redirect_edge_callee (struct cgraph_edge *, struct cgraph_node *);
-void cgraph_make_edge_direct (struct cgraph_edge *, struct cgraph_node *, tree);
+void cgraph_make_edge_direct (struct cgraph_edge *, struct cgraph_node *,
+			      HOST_WIDE_INT);
 
 struct cgraph_asm_node *cgraph_add_asm_node (tree);
 
Index: icln/gcc/cgraphunit.c
===================================================================
--- icln.orig/gcc/cgraphunit.c
+++ icln/gcc/cgraphunit.c
@@ -2168,22 +2168,18 @@  cgraph_redirect_edge_call_stmt_to_callee
 	}
     }
 
-  if (e->indirect_info && e->indirect_info->thunk_delta
-      && integer_nonzerop (e->indirect_info->thunk_delta)
+  if (e->thunk_delta != 0
       && (!e->callee->clone.combined_args_to_skip
 	  || !bitmap_bit_p (e->callee->clone.combined_args_to_skip, 0)))
     {
       if (cgraph_dump_file)
-	{
-	  fprintf (cgraph_dump_file, "          Thunk delta is ");
-	  print_generic_expr (cgraph_dump_file,
-			      e->indirect_info->thunk_delta, 0);
-	  fprintf (cgraph_dump_file, "\n");
-	}
+	fprintf (cgraph_dump_file, "          Thunk delta is "
+		 HOST_WIDE_INT_PRINT_DEC "\n", e->thunk_delta);
       gsi = gsi_for_stmt (e->call_stmt);
       gsi_computed = true;
-      gimple_adjust_this_by_delta (&gsi, e->indirect_info->thunk_delta);
-      e->indirect_info->thunk_delta = NULL_TREE;
+      gimple_adjust_this_by_delta (&gsi, build_int_cst (sizetype,
+							e->thunk_delta));
+      e->thunk_delta = 0;
     }
 
   if (e->callee->clone.combined_args_to_skip)
Index: icln/gcc/ipa-prop.c
===================================================================
--- icln.orig/gcc/ipa-prop.c
+++ icln/gcc/ipa-prop.c
@@ -1483,7 +1483,7 @@  ipa_make_edge_direct_to_target (struct c
     return NULL;
   ipa_check_create_node_params ();
 
-  cgraph_make_edge_direct (ie, callee, delta);
+  cgraph_make_edge_direct (ie, callee, delta ? tree_low_cst (delta, 0) : 0);
   if (dump_file)
     {
       fprintf (dump_file, "ipa-prop: Discovered %s call to a known target "
@@ -2549,7 +2549,6 @@  ipa_write_indirect_edge_info (struct out
     {
       lto_output_sleb128_stream (ob->main_stream, ii->otr_token);
       lto_output_tree (ob, ii->otr_type, true);
-      lto_output_tree (ob, ii->thunk_delta, true);
     }
 }
 
@@ -2572,7 +2571,6 @@  ipa_read_indirect_edge_info (struct lto_
     {
       ii->otr_token = (HOST_WIDE_INT) lto_input_sleb128 (ib);
       ii->otr_type = lto_input_tree (ib, data_in);
-      ii->thunk_delta = lto_input_tree (ib, data_in);
     }
 }
 
Index: icln/gcc/lto-cgraph.c
===================================================================
--- icln.orig/gcc/lto-cgraph.c
+++ icln/gcc/lto-cgraph.c
@@ -278,6 +278,7 @@  lto_output_edge (struct lto_simple_outpu
     }
 
   lto_output_sleb128_stream (ob->main_stream, edge->count);
+  lto_output_sleb128_stream (ob->main_stream, edge->thunk_delta);
 
   bp = bitpack_create (ob->main_stream);
   uid = (!gimple_has_body_p (edge->caller->decl)
@@ -1210,6 +1211,7 @@  input_edge (struct lto_input_block *ib,
   struct cgraph_edge *edge;
   unsigned int stmt_id;
   gcov_type count;
+  HOST_WIDE_INT thunk_delta;
   int freq;
   unsigned int nest;
   cgraph_inline_failed_t inline_failed;
@@ -1230,6 +1232,7 @@  input_edge (struct lto_input_block *ib,
     callee = NULL;
 
   count = (gcov_type) lto_input_sleb128 (ib);
+  thunk_delta = lto_input_sleb128 (ib);
 
   bp = lto_input_bitpack (ib);
   stmt_id = (unsigned int) bp_unpack_value (&bp, HOST_BITS_PER_INT);
@@ -1243,6 +1246,7 @@  input_edge (struct lto_input_block *ib,
   else
     edge = cgraph_create_edge (caller, callee, NULL, count, freq, nest);
 
+  edge->thunk_delta = thunk_delta;
   edge->indirect_inlining_edge = bp_unpack_value (&bp, 1);
   edge->lto_stmt_uid = stmt_id;
   edge->inline_failed = inline_failed;
Index: icln/gcc/testsuite/g++.dg/ipa/pr46984.C
===================================================================
--- /dev/null
+++ icln/gcc/testsuite/g++.dg/ipa/pr46984.C
@@ -0,0 +1,62 @@ 
+// { dg-options "-O -fipa-cp -fno-early-inlining -flto" }
+// { dg-do run }
+
+extern "C" void abort ();
+
+class A
+{
+public:
+  virtual void foo () {abort();}
+};
+
+class B : public A
+{
+public:
+  int z;
+  virtual void foo () {abort();}
+};
+
+class C : public A
+{
+public:
+  void *a[32];
+  unsigned long b;
+  long c[32];
+
+  virtual void foo () {abort();}
+};
+
+class D : public C, public B
+{
+public:
+  D () : C(), B()
+  {
+    int i;
+    for (i = 0; i < 32; i++)
+      {
+	a[i] = (void *) 0;
+	c[i] = 0;
+      }
+    b = 0xaaaa;
+  }
+
+  virtual void foo ();
+};
+
+void D::foo()
+{
+  if (b != 0xaaaa)
+    abort();
+}
+
+static inline void bar (B &b)
+{
+  b.foo ();
+}
+
+int main()
+{
+  D d;
+  bar (d);
+  return 0;
+}