Patchwork Fix early inliner inlining uninlinable functions

login
register
mail settings
Submitter Diego Novillo
Date Nov. 23, 2011, 5:50 p.m.
Message ID <CAD_=9DThnc5n8u7nV3JQe=nxx4qpKnm7BrXLL3xxhsxunxDUBw@mail.gmail.com>
Download mbox | patch
Permalink /patch/127360/
State New
Headers show

Comments

Diego Novillo - Nov. 23, 2011, 5:50 p.m.
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?


Diego.
Diego Novillo - Nov. 29, 2011, 1:44 p.m.
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.
IainS - Nov. 29, 2011, 1:50 p.m.
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
Diego Novillo - Nov. 29, 2011, 1:54 p.m.
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.
H.J. Lu - Nov. 29, 2011, 5:49 p.m.
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
Diego Novillo - Nov. 29, 2011, 5:57 p.m.
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.
Richard Guenther - Dec. 1, 2011, 10:03 a.m.
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
> 
>
Richard Guenther - Dec. 1, 2011, 10:55 a.m.
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.
Richard Guenther - Dec. 1, 2011, 10:59 a.m.
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.
Diego Novillo - Dec. 1, 2011, 11:54 a.m.
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.
Richard Guenther - Dec. 1, 2011, 12:08 p.m.
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.
Jan Hubicka - Dec. 1, 2011, 11:58 p.m.
>
> 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
Richard Guenther - Dec. 2, 2011, 9:53 a.m.
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.

Patch

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