| Message ID | 20130510172709.GH3568@virgil.suse |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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. */