| Message ID | CAD_=9DThnc5n8u7nV3JQe=nxx4qpKnm7BrXLL3xxhsxunxDUBw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Iain, could you let me know if the attached patch fixes your problem? The patch changes gimple_call_set_cannot_inline to update the corresponding callgraph edge, if needed. I did not touch any of the other calls, because sometimes we are calling this function in IPA mode, and so we don't know what function the call belongs to. I've tested it on x86_64. I will be committing it shortly. Thanks. Diego.
On 29 Nov 2011, at 13:44, Diego Novillo wrote:
> Iain, could you let me know if the attached patch fixes your problem?
apologies for not responding to the last message -
- Richi has already resolved the Ada issue as far as it affected
x86-64/darwin.
I assumed your message was addressed more generally to the cc list ...
.. happy to test the patch out, if you think relevant - or just keep
an eye for it hitting trunk.
cheers
Iain
On Tue, Nov 29, 2011 at 08:50, Iain Sandoe <developer@sandoe-acoustics.co.uk> wrote: > > On 29 Nov 2011, at 13:44, Diego Novillo wrote: > >> Iain, could you let me know if the attached patch fixes your problem? > > apologies for not responding to the last message - > - Richi has already resolved the Ada issue as far as it affected > x86-64/darwin. Ah, great. Then my patch was not fixing your issue. That's fine, thanks for confirming. Diego.
On Tue, Nov 29, 2011 at 5:44 AM, Diego Novillo <dnovillo@google.com> wrote: > Iain, could you let me know if the attached patch fixes your problem? > The patch changes gimple_call_set_cannot_inline to update the > corresponding callgraph edge, if needed. I did not touch any of the > other calls, because sometimes we are calling this function in IPA > mode, and so we don't know what function the call belongs to. > > I've tested it on x86_64. I will be committing it shortly. > This caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51346
On Tue, Nov 29, 2011 at 12:49, H.J. Lu <hjl.tools@gmail.com> wrote: > This caused: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51346 Thanks. I'm on it. Diego.
On Wed, 23 Nov 2011, Diego Novillo wrote: > On Sat, Nov 5, 2011 at 07:02, Iain Sandoe > <developer@sandoe-acoustics.co.uk> wrote: > > > > On 28 Oct 2011, at 13:57, Richard Guenther wrote: > > > >> > >> We fail to keep the cannot-inline flag up-to-date when turning > >> indirect to direct calls. The following patch arranges to do > >> this during statement folding (which should always be called > >> when that happens). It also makes sure to copy the updated flag > >> to the edge when iterating early inlining. > > > > This: http://gcc.gnu.org/ml/gcc-cvs/2011-11/msg00046.html > > > > regresses: > > acats/c740203a (x86-64-darwin10) > > gnat/aliasing3.adb (m64 i486-darwin9 and x86-64-darwin10) > > ... don't know about other platforms at present. > > I am also seeing a regression in some C++ code, specifically, this > call to gimple_call_set_cannot_inline() is not updating the > call_stmt_cannot_inline_p field in the corresponding call graph edge > > ! if (callee > ! && !gimple_check_call_matching_types (stmt, callee)) > ! gimple_call_set_cannot_inline (stmt, true); > > In this code I'm trying to build, we fail the assertion in can_inline_edge_p: > > /* Be sure that the cannot_inline_p flag is up to date. */ > gcc_checking_assert (!e->call_stmt > || (gimple_call_cannot_inline_p (e->call_stmt) > == e->call_stmt_cannot_inline_p) > > because gimple_fold_call did not update the inline flag on the edge. > > I grepped for calls to gimple_call_set_cannot_inline() and we don't > always bother to update the corresponding edge. I think the safest > approach here would be to make sure that we always do (patch below). > > Thoughts? Ick. Well. Which pass makes the flag change and why are edges not recomputed before inlining (they are, always!?). Well. It's a hack we have the flag duplicated. But the reason is we throw away the cgraph edges all the time (bah!) and at WPA time we don't have the stmt to lookup the flag. I'd rather remove the asserts than fixing up like this (btw, the inliner can handle all mismatches now). Richard. > > Diego. > > diff --git a/gcc/gimple.c b/gcc/gimple.c > index 071c651..e2b082a 100644 > --- a/gcc/gimple.c > +++ b/gcc/gimple.c > @@ -5558,4 +5558,31 @@ gimple_asm_clobbers_memory_p (const_gimple stmt) > > return false; > } > + > + > +/* Set the inlinable status of GIMPLE_CALL S to INLINABLE_P. */ > + > +void > +gimple_call_set_cannot_inline (gimple s, bool inlinable_p) > +{ > + bool prev_inlinable_p; > + > + GIMPLE_CHECK (s, GIMPLE_CALL); > + > + prev_inlinable_p = gimple_call_cannot_inline_p (s); > + > + if (inlinable_p) > + s->gsbase.subcode |= GF_CALL_CANNOT_INLINE; > + else > + s->gsbase.subcode &= ~GF_CALL_CANNOT_INLINE; > + > + if (prev_inlinable_p != inlinable_p) > + { > + struct cgraph_node *n = cgraph_get_node (current_function_decl); > + struct cgraph_edge *e = cgraph_edge (n, s); > + if (e) > + e->call_stmt_cannot_inline_p = inlinable_p; > + } > +} > + > #include "gt-gimple.h" > diff --git a/gcc/gimple.h b/gcc/gimple.h > index 8536c70..df31bf3 100644 > --- a/gcc/gimple.h > +++ b/gcc/gimple.h > @@ -1035,6 +1035,7 @@ extern bool walk_stmt_load_store_ops (gimple, void *, > extern bool gimple_ior_addresses_taken (bitmap, gimple); > extern bool gimple_call_builtin_p (gimple, enum built_in_function); > extern bool gimple_asm_clobbers_memory_p (const_gimple); > +extern void gimple_call_set_cannot_inline (gimple, bool); > > /* In gimplify.c */ > extern tree create_tmp_var_raw (tree, const char *); > @@ -2343,19 +2344,6 @@ gimple_call_tail_p (gimple s) > } > > > -/* Set the inlinable status of GIMPLE_CALL S to INLINABLE_P. */ > - > -static inline void > -gimple_call_set_cannot_inline (gimple s, bool inlinable_p) > -{ > - GIMPLE_CHECK (s, GIMPLE_CALL); > - if (inlinable_p) > - s->gsbase.subcode |= GF_CALL_CANNOT_INLINE; > - else > - s->gsbase.subcode &= ~GF_CALL_CANNOT_INLINE; > -} > - > - > /* Return true if GIMPLE_CALL S cannot be inlined. */ > > static inline bool > >
On Tue, 29 Nov 2011, Diego Novillo wrote: > Iain, could you let me know if the attached patch fixes your problem? > The patch changes gimple_call_set_cannot_inline to update the > corresponding callgraph edge, if needed. I did not touch any of the > other calls, because sometimes we are calling this function in IPA > mode, and so we don't know what function the call belongs to. > > I've tested it on x86_64. I will be committing it shortly. Btw, I don't think this "hammer" solution should be applied (what about the other direction?). Richard.
On Tue, 29 Nov 2011, Diego Novillo wrote: > On Tue, Nov 29, 2011 at 12:49, H.J. Lu <hjl.tools@gmail.com> wrote: > > > This caused: > > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51346 > > Thanks. I'm on it. The patch was wrong, please revert it. At the gimple stmt modification level we shouldn't modify the cgraph. That's a layering violation at least. Please file a bug with a reduced testcase that still fails without your fix. Richard.
On Thu, Dec 1, 2011 at 05:59, Richard Guenther <rguenther@suse.de> wrote: > On Tue, 29 Nov 2011, Diego Novillo wrote: > >> On Tue, Nov 29, 2011 at 12:49, H.J. Lu <hjl.tools@gmail.com> wrote: >> >> > This caused: >> > >> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51346 >> >> Thanks. I'm on it. > > The patch was wrong, please revert it. At the gimple stmt > modification level we shouldn't modify the cgraph. That's > a layering violation at least. No, this is a pre-existing problem that got aggravated with the new changes to the inline attribute in fold. I think we need to either toss out the edge attribute or make it such that they are more automatically sync'd. Unfortunately, we cannot get rid of it, since we sometimes do not have the statement. So, we have to live with the two attributes. How about, we make the edge attribute always dependent on the statement? If the statement exists, the edge attribute always take its value from it. Only when the statement doesn't exist, we take its value from the call. All this can be put into a small predicate. > Please file a bug with a reduced testcase that still fails > without your fix. I'll add a test to the final patch after it finishes reducing (the original test case is huge). Diego.
On Thu, 1 Dec 2011, Diego Novillo wrote: > On Thu, Dec 1, 2011 at 05:59, Richard Guenther <rguenther@suse.de> wrote: > > On Tue, 29 Nov 2011, Diego Novillo wrote: > > > >> On Tue, Nov 29, 2011 at 12:49, H.J. Lu <hjl.tools@gmail.com> wrote: > >> > >> > This caused: > >> > > >> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51346 > >> > >> Thanks. I'm on it. > > > > The patch was wrong, please revert it. At the gimple stmt > > modification level we shouldn't modify the cgraph. That's > > a layering violation at least. > > No, this is a pre-existing problem that got aggravated with the new > changes to the inline attribute in fold. I think we need to either > toss out the edge attribute or make it such that they are more > automatically sync'd. Unfortunately, we cannot get rid of it, since > we sometimes do not have the statement. > > So, we have to live with the two attributes. How about, we make the Yes. And I've fixed all places I could find sofar to update them. > edge attribute always dependent on the statement? If the statement > exists, the edge attribute always take its value from it. Only when > the statement doesn't exist, we take its value from the call. All > this can be put into a small predicate. Sure, but then you can still have the issue of an inconsistency. Thus, would you then remove the remaining asserts? I believe in the end the proper fix is to _not_ throw away cgraph edges all the time, but keep them up-to-date and thus make the stmt flag not necessary. (we can define "up-to-date" in a way so that we only require that existing edges that still have a call stmt have to be valid, thus still require incremental recomputation to remove dead edges and create new ones) > > Please file a bug with a reduced testcase that still fails > > without your fix. > > I'll add a test to the final patch after it finishes reducing (the > original test case is huge). Which pass did the folding of the stmt but did not adjust the edge flag? Richard.
> > Sure, but then you can still have the issue of an inconsistency. > Thus, would you then remove the remaining asserts? > > I believe in the end the proper fix is to _not_ throw away > cgraph edges all the time, but keep them up-to-date and thus > make the stmt flag not necessary. (we can define "up-to-date" > in a way so that we only require that existing edges that > still have a call stmt have to be valid, thus still require > incremental recomputation to remove dead edges and create > new ones) Well, the stmt flag always looked redundat to me. We we just don't initialize the edge flag at cgraph construction time? We do have the statement then. Honza
On Fri, 2 Dec 2011, Jan Hubicka wrote: > > > > Sure, but then you can still have the issue of an inconsistency. > > Thus, would you then remove the remaining asserts? > > > > I believe in the end the proper fix is to _not_ throw away > > cgraph edges all the time, but keep them up-to-date and thus > > make the stmt flag not necessary. (we can define "up-to-date" > > in a way so that we only require that existing edges that > > still have a call stmt have to be valid, thus still require > > incremental recomputation to remove dead edges and create > > new ones) > > Well, the stmt flag always looked redundat to me. We we just don't initialize > the edge flag at cgraph construction time? We do have the statement then. We've had the stmt flag because the gimplifier computed uninlinability and stuck it on the CALL_EXPR tree, then transitioned it to the gimple stmt. We no longer do that, so yes - I'll prepare a patch to kill all this after Diego committed his patch ;) Richard.
diff --git a/gcc/gimple.c b/gcc/gimple.c index 071c651..e2b082a 100644 --- a/gcc/gimple.c +++ b/gcc/gimple.c @@ -5558,4 +5558,31 @@ gimple_asm_clobbers_memory_p (const_gimple stmt) return false; } + + +/* Set the inlinable status of GIMPLE_CALL S to INLINABLE_P. */ + +void +gimple_call_set_cannot_inline (gimple s, bool inlinable_p) +{ + bool prev_inlinable_p; + + GIMPLE_CHECK (s, GIMPLE_CALL); + + prev_inlinable_p = gimple_call_cannot_inline_p (s); + + if (inlinable_p) + s->gsbase.subcode |= GF_CALL_CANNOT_INLINE; + else + s->gsbase.subcode &= ~GF_CALL_CANNOT_INLINE; + + if (prev_inlinable_p != inlinable_p) + { + struct cgraph_node *n = cgraph_get_node (current_function_decl); + struct cgraph_edge *e = cgraph_edge (n, s); + if (e) + e->call_stmt_cannot_inline_p = inlinable_p; + } +} + #include "gt-gimple.h" diff --git a/gcc/gimple.h b/gcc/gimple.h index 8536c70..df31bf3 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -1035,6 +1035,7 @@ extern bool walk_stmt_load_store_ops (gimple, void *, extern bool gimple_ior_addresses_taken (bitmap, gimple); extern bool gimple_call_builtin_p (gimple, enum built_in_function); extern bool gimple_asm_clobbers_memory_p (const_gimple); +extern void gimple_call_set_cannot_inline (gimple, bool); /* In gimplify.c */ extern tree create_tmp_var_raw (tree, const char *); @@ -2343,19 +2344,6 @@ gimple_call_tail_p (gimple s) } -/* Set the inlinable status of GIMPLE_CALL S to INLINABLE_P. */ - -static inline void -gimple_call_set_cannot_inline (gimple s, bool inlinable_p) -{ - GIMPLE_CHECK (s, GIMPLE_CALL); - if (inlinable_p) - s->gsbase.subcode |= GF_CALL_CANNOT_INLINE; - else - s->gsbase.subcode &= ~GF_CALL_CANNOT_INLINE; -} - - /* Return true if GIMPLE_CALL S cannot be inlined. */ static inline bool