Patchwork Redirect calls to non-functions to builtin_unreachable

login
register
mail settings
Submitter Martin Jambor
Date May 10, 2013, 5:27 p.m.
Message ID <20130510172709.GH3568@virgil.suse>
Download mbox | patch
Permalink /patch/243046/
State New
Headers show

Comments

Martin Jambor - May 10, 2013, 5:27 p.m.
Hi,

as we discover targets of previously indirect calls in ipa-inline and
ipa-cp, we sometimes figure out that the targets are not a function.
One typical example is when NULL is passed as a function pointer
parameter, another is when C++ member-pointer points to a virtual
function and the overloaded field of the structure which can also hold
pointers to non-virtual methods contains odd integer constants.

I have gone through all the cases when this happens when LTO building
Mozilla Firefox and verified all of them were OK and legal.  Because
no such call to a non-function can ever occur in a legal program, I
have decided to turn them into calls of builting_unreachable, which
then could help further optimizations.

Because calls to builtin_unreachable is not cgraph_maybe_hot_edge_p,
whereas an indirect call with an unknown destination is, this change
often triggered assertion tree-ipa-inline-transform.c:263.  I have
discussed this with Honza and he has agreed to switch the check off
when there are newly discovered direct edges.

The patch has passed bootstrap and is undergoing testing now.  OK for
trunk if it passes?

Thanks,

Martin


2013-05-09  Martin Jambor  <mjambor@suse.cz>

	* ipa-prop.c (ipa_make_edge_direct_to_target): Redirect calls to
	non-functions to builtin_unreachable.
	* ipa-inline-transform.c (inline_call): Do not assert estimates were
	correct when new direct edges were discovered.
Jeff Law - May 10, 2013, 5:43 p.m.
On 05/10/2013 11:27 AM, Martin Jambor wrote:
> Hi,
>
> as we discover targets of previously indirect calls in ipa-inline and
> ipa-cp, we sometimes figure out that the targets are not a function.
> One typical example is when NULL is passed as a function pointer
> parameter, another is when C++ member-pointer points to a virtual
> function and the overloaded field of the structure which can also hold
> pointers to non-virtual methods contains odd integer constants.
>
> I have gone through all the cases when this happens when LTO building
> Mozilla Firefox and verified all of them were OK and legal.  Because
> no such call to a non-function can ever occur in a legal program, I
> have decided to turn them into calls of builting_unreachable, which
> then could help further optimizations.
>
> Because calls to builtin_unreachable is not cgraph_maybe_hot_edge_p,
> whereas an indirect call with an unknown destination is, this change
> often triggered assertion tree-ipa-inline-transform.c:263.  I have
> discussed this with Honza and he has agreed to switch the check off
> when there are newly discovered direct edges.
>
> The patch has passed bootstrap and is undergoing testing now.  OK for
> trunk if it passes?
>
> Thanks,
>
> Martin
>
>
> 2013-05-09  Martin Jambor  <mjambor@suse.cz>
>
> 	* ipa-prop.c (ipa_make_edge_direct_to_target): Redirect calls to
> 	non-functions to builtin_unreachable.
> 	* ipa-inline-transform.c (inline_call): Do not assert estimates were
> 	correct when new direct edges were discovered.
This is good.  Please install.

Thanks,
Jeff
Marcus Shawcroft - June 20, 2013, 2:47 p.m.
Hi,  I've been looking at an issue in mysql compilation which appears
to be due to this patch.

On 10 May 2013 18:27, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> as we discover targets of previously indirect calls in ipa-inline and
> ipa-cp, we sometimes figure out that the targets are not a function.
> One typical example is when NULL is passed as a function pointer

OK, this part makes sense to me.

> parameter, another is when C++ member-pointer points to a virtual
> function and the overloaded field of the structure which can also hold
> pointers to non-virtual methods contains odd integer constants.

I'm struggling to understand why such a member-pointer call would be
illegal in a well formed program.

Attached is a fragment of code that demonstrates the issue I've been
looking at.  When compiled at -O3 the 047i.inline dump tells me that:

ipa-prop: Discovered direct call to non-function in unsigned int
A::foo(unsigned int (H::*)() const) const/11, making it unreachable.
  not inlinable: unsigned int A::foo(unsigned int (H::*)() const)
const/11 -> void __builtin_unreachable()/12, function body not
available

This behavior appears to be the explicit intent of the original patch,
the call to the member function pointer has been replaced with
__builtin_unreachable, but that looks like a legitimate  call to me.
What am I missing?

Thanks
/Marcus
Martin Jambor - June 20, 2013, 3:46 p.m.
sHi,

On Thu, Jun 20, 2013 at 03:47:11PM +0100, Marcus Shawcroft wrote:
> Hi,  I've been looking at an issue in mysql compilation which appears
> to be due to this patch.
> 
> On 10 May 2013 18:27, Martin Jambor <mjambor@suse.cz> wrote:
> > Hi,
> >
> > as we discover targets of previously indirect calls in ipa-inline and
> > ipa-cp, we sometimes figure out that the targets are not a function.
> > One typical example is when NULL is passed as a function pointer
> 
> OK, this part makes sense to me.
> 
> > parameter, another is when C++ member-pointer points to a virtual
> > function and the overloaded field of the structure which can also hold
> > pointers to non-virtual methods contains odd integer constants.
> 
> I'm struggling to understand why such a member-pointer call would be
> illegal in a well formed program.
> 
> Attached is a fragment of code that demonstrates the issue I've been
> looking at.  When compiled at -O3 the 047i.inline dump tells me that:
> 
> ipa-prop: Discovered direct call to non-function in unsigned int
> A::foo(unsigned int (H::*)() const) const/11, making it unreachable.
>   not inlinable: unsigned int A::foo(unsigned int (H::*)() const)
> const/11 -> void __builtin_unreachable()/12, function body not
> available
> 
> This behavior appears to be the explicit intent of the original patch,
> the call to the member function pointer has been replaced with
> __builtin_unreachable, but that looks like a legitimate  call to me.
> What am I missing?

Hm, the reason why I did this was that I misremembered that each
branch, the one for virtual calls and the one for direct calls.  But
when I checked dumps, I realized it was not so, there is only one call
when the branches join.  I'll fix this promptly.

Thanks for the report,

Martin
Martin Jambor - June 24, 2013, 9:51 p.m.
Hi,

On Thu, Jun 20, 2013 at 05:46:28PM +0200, Martin Jambor wrote:
> On Thu, Jun 20, 2013 at 03:47:11PM +0100, Marcus Shawcroft wrote:
> > Hi,  I've been looking at an issue in mysql compilation which appears
> > to be due to this patch.
> > 
> > On 10 May 2013 18:27, Martin Jambor <mjambor@suse.cz> wrote:
> > > Hi,
> > >
> > > as we discover targets of previously indirect calls in ipa-inline and
> > > ipa-cp, we sometimes figure out that the targets are not a function.
> > > One typical example is when NULL is passed as a function pointer
> > 
> > OK, this part makes sense to me.
> > 
> > > parameter, another is when C++ member-pointer points to a virtual
> > > function and the overloaded field of the structure which can also hold
> > > pointers to non-virtual methods contains odd integer constants.
> > 
> > I'm struggling to understand why such a member-pointer call would be
> > illegal in a well formed program.
> > 
> > Attached is a fragment of code that demonstrates the issue I've been
> > looking at.  When compiled at -O3 the 047i.inline dump tells me that:
> > 
> > ipa-prop: Discovered direct call to non-function in unsigned int
> > A::foo(unsigned int (H::*)() const) const/11, making it unreachable.
> >   not inlinable: unsigned int A::foo(unsigned int (H::*)() const)
> > const/11 -> void __builtin_unreachable()/12, function body not
> > available
> > 
> > This behavior appears to be the explicit intent of the original patch,
> > the call to the member function pointer has been replaced with
> > __builtin_unreachable, but that looks like a legitimate  call to me.
> > What am I missing?
> 
> Hm, the reason why I did this was that I misremembered that each
> branch, the one for virtual calls and the one for direct calls.  But
> when I checked dumps, I realized it was not so, there is only one call
> when the branches join.  I'll fix this promptly.
> 
> Thanks for the report,

For the record, this is PR 57670.

Martin

Patch

Index: src/gcc/ipa-prop.c
===================================================================
--- src.orig/gcc/ipa-prop.c
+++ src/gcc/ipa-prop.c
@@ -2200,6 +2200,7 @@  ipa_make_edge_direct_to_target (struct c
 {
   struct cgraph_node *callee;
   struct inline_edge_summary *es = inline_edge_summary (ie);
+  bool unreachable = false;
 
   if (TREE_CODE (target) == ADDR_EXPR)
     target = TREE_OPERAND (target, 0);
@@ -2210,12 +2211,17 @@  ipa_make_edge_direct_to_target (struct c
 	{
 	  if (dump_file)
 	    fprintf (dump_file, "ipa-prop: Discovered direct call to non-function"
-				" in %s/%i.\n",
+				" in %s/%i, making it unreachable.\n",
 		     cgraph_node_name (ie->caller), ie->caller->symbol.order);
-	  return NULL;
+	  target = builtin_decl_implicit (BUILT_IN_UNREACHABLE);
+	  callee = cgraph_get_create_node (target);
+	  unreachable = true;
 	}
+      else
+	callee = cgraph_get_node (target);
     }
-  callee = cgraph_get_node (target);
+  else
+    callee = cgraph_get_node (target);
 
   /* Because may-edges are not explicitely represented and vtable may be external,
      we may create the first reference to the object in the unit.  */
@@ -2252,7 +2258,7 @@  ipa_make_edge_direct_to_target (struct c
 			 - eni_size_weights.call_cost);
   es->call_stmt_time -= (eni_time_weights.indirect_call_cost
 			 - eni_time_weights.call_cost);
-  if (dump_file)
+  if (dump_file && !unreachable)
     {
       fprintf (dump_file, "ipa-prop: Discovered %s call to a known target "
 	       "(%s/%i -> %s/%i), for stmt ",
Index: src/gcc/ipa-inline-transform.c
===================================================================
--- src.orig/gcc/ipa-inline-transform.c
+++ src/gcc/ipa-inline-transform.c
@@ -260,7 +260,7 @@  inline_call (struct cgraph_edge *e, bool
 #ifdef ENABLE_CHECKING
   /* Verify that estimated growth match real growth.  Allow off-by-one
      error due to INLINE_SIZE_SCALE roudoff errors.  */
-  gcc_assert (!update_overall_summary || !overall_size
+  gcc_assert (!update_overall_summary || !overall_size || new_edges_found
 	      || abs (estimated_growth - (new_size - old_size)) <= 1
 	      /* FIXME: a hack.  Edges with false predicate are accounted
 		 wrong, we should remove them from callgraph.  */