Patchwork Fix inlining glitch

login
register
mail settings
Submitter Eric Botcazou
Date July 24, 2011, 5:12 p.m.
Message ID <201107241912.51760.ebotcazou@adacore.com>
Download mbox | patch
Permalink /patch/106534/
State New
Headers show

Comments

Eric Botcazou - July 24, 2011, 5:12 p.m.
Hi,

we sometimes get messages like this in Ada:

prime-mc2-other.adb: In function 'PRIME.MC2.OTHER.DO_SOMETHING':
prime-mc2.adb:2:4: warning: inlining failed in call 
to 'PRIME.MC2.GET_INPUT_VALUE.PART': non-call exception handling mismatch 
[-Winline]
prime-mc2-other.adb:3:4: warning: called from here [-Winline]

Since this is for a pure Ada program, it's unexpected.  This stems from virtual 
cloning: cgraph_create_virtual_clone creates the virtual clone and does:

  DECL_STRUCT_FUNCTION (new_decl) = NULL;

so the can_throw_non_call_exceptions flag isn't preserved and can_inline_edge_p 
is fooled into thinking that it cannot inline.

It's probably better not to fiddle with virtual cloning so the attached patch 
teaches can_inline_edge_p to look into DECL_STRUCT_FUNCTION of the original 
nodes if it is dealing with virtual clones.

Tested on i586-suse-linux, OK for the mainline?


2011-07-24  Eric Botcazou  <ebotcazou@adacore.com>

	* ipa-inline.c (can_inline_edge_p): Look into DECL_STRUCT_FUNCTION of
	original nodes if we are dealing with virtual clones.
Richard Guenther - July 25, 2011, 9:02 a.m.
On Sun, Jul 24, 2011 at 7:12 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> we sometimes get messages like this in Ada:
>
> prime-mc2-other.adb: In function 'PRIME.MC2.OTHER.DO_SOMETHING':
> prime-mc2.adb:2:4: warning: inlining failed in call
> to 'PRIME.MC2.GET_INPUT_VALUE.PART': non-call exception handling mismatch
> [-Winline]
> prime-mc2-other.adb:3:4: warning: called from here [-Winline]
>
> Since this is for a pure Ada program, it's unexpected.  This stems from virtual
> cloning: cgraph_create_virtual_clone creates the virtual clone and does:
>
>  DECL_STRUCT_FUNCTION (new_decl) = NULL;
>
> so the can_throw_non_call_exceptions flag isn't preserved and can_inline_edge_p
> is fooled into thinking that it cannot inline.
>
> It's probably better not to fiddle with virtual cloning so the attached patch
> teaches can_inline_edge_p to look into DECL_STRUCT_FUNCTION of the original
> nodes if it is dealing with virtual clones.
>
> Tested on i586-suse-linux, OK for the mainline?

Doesn't cgraph_function_or_thunk_node already deal with this?  Honza?

Richard.

>
> 2011-07-24  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * ipa-inline.c (can_inline_edge_p): Look into DECL_STRUCT_FUNCTION of
>        original nodes if we are dealing with virtual clones.
>
>
> --
> Eric Botcazou
>
Jan Hubicka - July 25, 2011, 9:44 a.m.
> On Sun, Jul 24, 2011 at 7:12 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> > Hi,
> >
> > we sometimes get messages like this in Ada:
> >
> > prime-mc2-other.adb: In function 'PRIME.MC2.OTHER.DO_SOMETHING':
> > prime-mc2.adb:2:4: warning: inlining failed in call
> > to 'PRIME.MC2.GET_INPUT_VALUE.PART': non-call exception handling mismatch
> > [-Winline]
> > prime-mc2-other.adb:3:4: warning: called from here [-Winline]
> >
> > Since this is for a pure Ada program, it's unexpected.  This stems from virtual
> > cloning: cgraph_create_virtual_clone creates the virtual clone and does:
> >
> >  DECL_STRUCT_FUNCTION (new_decl) = NULL;
> >
> > so the can_throw_non_call_exceptions flag isn't preserved and can_inline_edge_p
> > is fooled into thinking that it cannot inline.
> >
> > It's probably better not to fiddle with virtual cloning so the attached patch
> > teaches can_inline_edge_p to look into DECL_STRUCT_FUNCTION of the original
> > nodes if it is dealing with virtual clones.
> >
> > Tested on i586-suse-linux, OK for the mainline?
> 
> Doesn't cgraph_function_or_thunk_node already deal with this?  Honza?

No, the problem here is deciding whether we can inline a clone.
We look into DECL_STRUCT_FUNCTION that we can't.  The real fix is one commented in:

  /* Don't inline if the callee can throw non-call exceptions but the
     caller cannot.
     FIXME: this is obviously wrong for LTO where STRUCT_FUNCTION is missing.
     Move the flag into cgraph node or mirror it in the inline summary.  */

I plan to look into this before next release.  I would, for sure, welcome Eric beating me. If he don't have
time to do so, I think the patch is OK as it is, since it improves the situation despite the fact that
it won't fix the same problem with WPA.

Honza
> 
> Richard.
> 
> >
> > 2011-07-24  Eric Botcazou  <ebotcazou@adacore.com>
> >
> >        * ipa-inline.c (can_inline_edge_p): Look into DECL_STRUCT_FUNCTION of
> >        original nodes if we are dealing with virtual clones.
> >
> >
> > --
> > Eric Botcazou
> >
Eric Botcazou - July 25, 2011, 10:07 a.m.
> No, the problem here is deciding whether we can inline a clone.
> We look into DECL_STRUCT_FUNCTION that we can't.  The real fix is one
> commented in:
>
>   /* Don't inline if the callee can throw non-call exceptions but the
>      caller cannot.
>      FIXME: this is obviously wrong for LTO where STRUCT_FUNCTION is
> missing. Move the flag into cgraph node or mirror it in the inline summary.
>  */

The irony being that I implemented the flag for the sake of LTO, based on 
suggestions made on this list...  So why is STRUCT_FUNCTION missing now?

> I plan to look into this before next release.  I would, for sure, welcome
> Eric beating me. If he don't have time to do so, I think the patch is OK as
> it is, since it improves the situation despite the fact that it won't fix
> the same problem with WPA.

OK, I'll install the patch for now.
Jan Hubicka - July 25, 2011, 10:26 a.m.
> > No, the problem here is deciding whether we can inline a clone.
> > We look into DECL_STRUCT_FUNCTION that we can't.  The real fix is one
> > commented in:
> >
> >   /* Don't inline if the callee can throw non-call exceptions but the
> >      caller cannot.
> >      FIXME: this is obviously wrong for LTO where STRUCT_FUNCTION is
> > missing. Move the flag into cgraph node or mirror it in the inline summary.
> >  */
> 
> The irony being that I implemented the flag for the sake of LTO, based on 
> suggestions made on this list...  So why is STRUCT_FUNCTION missing now?

I noticed that, sorry it was also my ommision.

DECL_STRUCT_FUNCTION and function bodies are not load into WPA stage, only into
ltrans. This is how WHOPR is designed.

WPA stage is expected to work across cgraph/varpool and not look into the
bodies/initializers.  So this flag, as well as the optimization settings, needs
to be copied there. 

Since we do a lot of querries from codegen to this particular flag, I would
suggest to simply copy it into inline_summary and make inline_analyze_function
to copy it rather than moving it to place less convenient for the codegen.

Honza
> 
> > I plan to look into this before next release.  I would, for sure, welcome
> > Eric beating me. If he don't have time to do so, I think the patch is OK as
> > it is, since it improves the situation despite the fact that it won't fix
> > the same problem with WPA.
> 
> OK, I'll install the patch for now.
> 
> -- 
> Eric Botcazou

Patch

Index: ipa-inline.c
===================================================================
--- ipa-inline.c	(revision 176622)
+++ ipa-inline.c	(working copy)
@@ -238,9 +238,20 @@  can_inline_edge_p (struct cgraph_edge *e
 {
   bool inlinable = true;
   enum availability avail;
-  struct cgraph_node *callee = cgraph_function_or_thunk_node (e->callee, &avail);
+  struct cgraph_node *callee
+    = cgraph_function_or_thunk_node (e->callee, &avail);
   tree caller_tree = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (e->caller->decl);
-  tree callee_tree = callee ? DECL_FUNCTION_SPECIFIC_OPTIMIZATION (callee->decl) : NULL;
+  tree callee_tree
+    = callee ? DECL_FUNCTION_SPECIFIC_OPTIMIZATION (callee->decl) : NULL;
+  struct function *caller_cfun = DECL_STRUCT_FUNCTION (e->caller->decl);
+  struct function *callee_cfun
+    = callee ? DECL_STRUCT_FUNCTION (callee->decl) : NULL;
+
+  if (!caller_cfun && e->caller->clone_of)
+    caller_cfun = DECL_STRUCT_FUNCTION (e->caller->clone_of->decl);
+
+  if (!callee_cfun && callee && callee->clone_of)
+    callee_cfun = DECL_STRUCT_FUNCTION (callee->clone_of->decl);
 
   gcc_assert (e->inline_failed);
 
@@ -277,12 +288,8 @@  can_inline_edge_p (struct cgraph_edge *e
      caller cannot.
      FIXME: this is obviously wrong for LTO where STRUCT_FUNCTION is missing.
      Move the flag into cgraph node or mirror it in the inline summary.  */
-  else if (DECL_STRUCT_FUNCTION (callee->decl)
-	   && DECL_STRUCT_FUNCTION
-	        (callee->decl)->can_throw_non_call_exceptions
-	   && !(DECL_STRUCT_FUNCTION (e->caller->decl)
-	        && DECL_STRUCT_FUNCTION
-		     (e->caller->decl)->can_throw_non_call_exceptions))
+  else if (callee_cfun && callee_cfun->can_throw_non_call_exceptions
+	   && !(caller_cfun && caller_cfun->can_throw_non_call_exceptions))
     {
       e->inline_failed = CIF_NON_CALL_EXCEPTIONS;
       inlinable = false;