[patch] Restore cross-language inlining into Ada

[patch] Restore cross-language inlining into Ada

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

Hi,

this patch from Jan:
  https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01388.html
totally disabled cross-language inlining into Ada without notice, by adding a 
check that always fails when the language of the callee is not Ada...
The attached patch simply deletes this new check to restore the initial state.

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


2016-01-20  Eric Botcazou  <ebotcazou@adacore.com>

	* ipa-inline.c (can_inline_edge_p): Back out overzealous check on
	flag_non_call_exceptions compatibility.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 868 bytes --]

Index: ipa-inline.c
===================================================================
--- ipa-inline.c	(revision 232465)
+++ ipa-inline.c	(working copy)
@@ -432,11 +432,7 @@ can_inline_edge_p (struct cgraph_edge *e
 		 does not throw.
 		 This is tracked by DECL_FUNCTION_PERSONALITY.  */
 	      || (check_match (flag_non_call_exceptions)
-		  /* TODO: We also may allow bringing !flag_non_call_exceptions
-		     to flag_non_call_exceptions function, but that may need
-		     extra work in tree-inline to add the extra EH edges.  */
-		  && (!opt_for_fn (callee->decl, flag_non_call_exceptions)
-		      || DECL_FUNCTION_PERSONALITY (callee->decl)))
+		  && DECL_FUNCTION_PERSONALITY (callee->decl))
 	      || (check_maybe_up (flag_exceptions)
 		  && DECL_FUNCTION_PERSONALITY (callee->decl))
 	      /* Strictly speaking only when the callee contains function

Re: [patch] Restore cross-language inlining into Ada

[Richard Biener]

On Wed, Jan 20, 2016 at 9:32 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this patch from Jan:
>   https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01388.html
> totally disabled cross-language inlining into Ada without notice, by adding a
> check that always fails when the language of the callee is not Ada...
> The attached patch simply deletes this new check to restore the initial state.
>
> Tested on x86_64-suse-linux, OK for the mainline?

I think the intent was to allow inlining a non-throwing -fnon-call-exceptions
function into a not -fnon-call-exceptions function but _not_ a
non-throwing not -fnon-call-exceptions function (that "not-throwing" is
basically a non-sensible test) into a -fnon-call-exceptions function
because that may now miss EH edges.

So the test looks conservatively correct to me - we can't reliably
check whether the callee throws if the IL now were -fnon-call-exceptions
(which we know the caller is after !opt_for_fn (callee->decl,
flag_non_call_exceptions)

So - this doesn't look correct to me.

OTOH

static inline int foo (int a, int *b)
{
  return a / *b;
}

int __attribute__((optimize("non-call-exceptions")))
bar (int *p, int *b)
{
  try
    {
      return foo (*p, b);
    }
  catch (...)
    {
      return 0;
    }
}

happily inlines foo with your patch but doesn't ICE during stmt verification.

So maybe we're not verifying that "correctness" part - ah, yeah, I think
we changed it to only verify EH tree vs. stmt consistency but not the
other way around.

Not sure if we already have a C++ testcase like the above, if not can
you add this one to the torture?

Given this I wonder if we can also change check_match to check_maybe_up,
basically handle -fnon-call-exceptions the same as -fexceptions.

Thanks,
Richard.

>
> 2016-01-20  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * ipa-inline.c (can_inline_edge_p): Back out overzealous check on
>         flag_non_call_exceptions compatibility.
>
>
> --
> Eric Botcazou

Re: [patch] Restore cross-language inlining into Ada

[Jan Hubicka]

> On Wed, Jan 20, 2016 at 9:32 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> > Hi,
> >
> > this patch from Jan:
> >   https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01388.html
> > totally disabled cross-language inlining into Ada without notice, by adding a
> > check that always fails when the language of the callee is not Ada...
> > The attached patch simply deletes this new check to restore the initial state.

I only updated
-  /* 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.  */
-  else if (callee_fun && callee_fun->can_throw_non_call_exceptions
-	   && !(caller_fun && caller_fun->can_throw_non_call_exceptions))
-    {
-      e->inline_failed = CIF_NON_CALL_EXCEPTIONS;
-      inlinable = false;
-    }
to actually work with LTO where callee_fun/caller_fun is not always available
(but sometimes, like when ICF requested the body or when we merged profiles, it
is).

> >
> > Tested on x86_64-suse-linux, OK for the mainline?
> 
> I think the intent was to allow inlining a non-throwing -fnon-call-exceptions
> function into a not -fnon-call-exceptions function but _not_ a
> non-throwing not -fnon-call-exceptions function (that "not-throwing" is
> basically a non-sensible test) into a -fnon-call-exceptions function
> because that may now miss EH edges.
> 
> So the test looks conservatively correct to me - we can't reliably
> check whether the callee throws if the IL now were -fnon-call-exceptions
> (which we know the caller is after !opt_for_fn (callee->decl,
> flag_non_call_exceptions)
> 
> So - this doesn't look correct to me.
> 
> OTOH
> 
> static inline int foo (int a, int *b)
> {
>   return a / *b;
> }
> 
> int __attribute__((optimize("non-call-exceptions")))
> bar (int *p, int *b)
> {
>   try
>     {
>       return foo (*p, b);
>     }
>   catch (...)
>     {
>       return 0;
>     }
> }
> 
> happily inlines foo with your patch but doesn't ICE during stmt verification.
> 
> So maybe we're not verifying that "correctness" part - ah, yeah, I think
> we changed it to only verify EH tree vs. stmt consistency but not the
> other way around.

Well, it is a while since I looked deeper into EH code, but if I remember
correctly we have EH region associated with statements and the non-call
exceptions do not have EH region that is taken by EH code as an information
that the statement was proved to not throw? In that case inlining could be
safe, if the inlined statements are not placed in EH region (I think inliner
does that)

So perhaps this inlining is always safe?

Honza

Re: [patch] Restore cross-language inlining into Ada

[Richard Biener]

On Thu, Jan 21, 2016 at 3:13 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Wed, Jan 20, 2016 at 9:32 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> > Hi,
>> >
>> > this patch from Jan:
>> >   https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01388.html
>> > totally disabled cross-language inlining into Ada without notice, by adding a
>> > check that always fails when the language of the callee is not Ada...
>> > The attached patch simply deletes this new check to restore the initial state.
>
> I only updated
> -  /* 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.  */
> -  else if (callee_fun && callee_fun->can_throw_non_call_exceptions
> -          && !(caller_fun && caller_fun->can_throw_non_call_exceptions))
> -    {
> -      e->inline_failed = CIF_NON_CALL_EXCEPTIONS;
> -      inlinable = false;
> -    }
> to actually work with LTO where callee_fun/caller_fun is not always available
> (but sometimes, like when ICF requested the body or when we merged profiles, it
> is).
>
>> >
>> > Tested on x86_64-suse-linux, OK for the mainline?
>>
>> I think the intent was to allow inlining a non-throwing -fnon-call-exceptions
>> function into a not -fnon-call-exceptions function but _not_ a
>> non-throwing not -fnon-call-exceptions function (that "not-throwing" is
>> basically a non-sensible test) into a -fnon-call-exceptions function
>> because that may now miss EH edges.
>>
>> So the test looks conservatively correct to me - we can't reliably
>> check whether the callee throws if the IL now were -fnon-call-exceptions
>> (which we know the caller is after !opt_for_fn (callee->decl,
>> flag_non_call_exceptions)
>>
>> So - this doesn't look correct to me.
>>
>> OTOH
>>
>> static inline int foo (int a, int *b)
>> {
>>   return a / *b;
>> }
>>
>> int __attribute__((optimize("non-call-exceptions")))
>> bar (int *p, int *b)
>> {
>>   try
>>     {
>>       return foo (*p, b);
>>     }
>>   catch (...)
>>     {
>>       return 0;
>>     }
>> }
>>
>> happily inlines foo with your patch but doesn't ICE during stmt verification.
>>
>> So maybe we're not verifying that "correctness" part - ah, yeah, I think
>> we changed it to only verify EH tree vs. stmt consistency but not the
>> other way around.
>
> Well, it is a while since I looked deeper into EH code, but if I remember
> correctly we have EH region associated with statements and the non-call
> exceptions do not have EH region that is taken by EH code as an information
> that the statement was proved to not throw? In that case inlining could be
> safe, if the inlined statements are not placed in EH region (I think inliner
> does that)
>
> So perhaps this inlining is always safe?

That's what I think.

Richard.

> Honza

Re: [patch] Restore cross-language inlining into Ada

[Jan Hubicka]

> >
> > Well, it is a while since I looked deeper into EH code, but if I remember
> > correctly we have EH region associated with statements and the non-call
> > exceptions do not have EH region that is taken by EH code as an information
> > that the statement was proved to not throw? In that case inlining could be
> > safe, if the inlined statements are not placed in EH region (I think inliner
> > does that)
> >
> > So perhaps this inlining is always safe?
> 
> That's what I think.
OK, as far as I can tell, Eric introduced the check in 
https://gcc.gnu.org/ml/gcc-patches/2010-05/msg01822.html

I am fine with it being relaxed and permitting inlining !non_call_exceptions to
non_call_exceptions functions..  It would be also cool to have a testcases.

Honza

Re: [patch] Restore cross-language inlining into Ada

[Eric Botcazou]

> I am fine with it being relaxed and permitting inlining !non_call_exceptions
> to non_call_exceptions functions..  It would be also cool to have a
> testcases.

Thanks, patch installed with check_match changed to check_maybe_up, I'll work 
towards adding an Ada+C LTO test but this will require fiddling with DejaGNU.

-- 
Eric Botcazou

Re: [patch] Restore cross-language inlining into Ada

[Richard Biener]

On Fri, Jan 22, 2016 at 12:06 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I am fine with it being relaxed and permitting inlining !non_call_exceptions
>> to non_call_exceptions functions..  It would be also cool to have a
>> testcases.
>
> Thanks, patch installed with check_match changed to check_maybe_up, I'll work
> towards adding an Ada+C LTO test but this will require fiddling with DejaGNU.

I suppose that some C++ mixed -f[no-]non-call-exceptions testcases
might be good enough.

Richard.

> --
> Eric Botcazou

Re: [patch] Restore cross-language inlining into Ada

[Eric Botcazou]

> I only updated
> -  /* 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.  */ -  else if (callee_fun &&
> callee_fun->can_throw_non_call_exceptions -	   && !(caller_fun &&
> caller_fun->can_throw_non_call_exceptions)) -    {
> -      e->inline_failed = CIF_NON_CALL_EXCEPTIONS;
> -      inlinable = false;
> -    }
> to actually work with LTO where callee_fun/caller_fun is not always
> available (but sometimes, like when ICF requested the body or when we
> merged profiles, it is).

No, that's not true.  Let's consider an Ada caller and a C callee.  With the 
old code (mine as you remarked): caller_fun->can_throw_non_call_exceptions is 
true and callee_fun->can_throw_non_call_exceptions is false, so the above test 
is false and we can inline.  With the new code (yours): check_match is true 
and opt_for_fn (callee->decl, flag_non_call_exceptions) is false, so we cannot 
inline.

-- 
Eric Botcazou

Re: [patch] Restore cross-language inlining into Ada

[Jan Hubicka]

> > I only updated
> > -  /* 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.  */ -  else if (callee_fun &&
> > callee_fun->can_throw_non_call_exceptions -	   && !(caller_fun &&
> > caller_fun->can_throw_non_call_exceptions)) -    {
> > -      e->inline_failed = CIF_NON_CALL_EXCEPTIONS;
> > -      inlinable = false;
> > -    }
> > to actually work with LTO where callee_fun/caller_fun is not always
> > available (but sometimes, like when ICF requested the body or when we
> > merged profiles, it is).
> 
> No, that's not true.  Let's consider an Ada caller and a C callee.  With the 
> old code (mine as you remarked): caller_fun->can_throw_non_call_exceptions is 
> true and callee_fun->can_throw_non_call_exceptions is false, so the above test 
> is false and we can inline.  With the new code (yours): check_match is true 
> and opt_for_fn (callee->decl, flag_non_call_exceptions) is false, so we cannot 
> inline.

Hmm, I see now.  I wonder if we can also inline can_thorw_non_call_exceptions
to !can_throw_non_call_exceptions provied that we set the flag in
ipa-inline-transform.  That way we can inline Ada to C and the observation
about no EH regions should still hold.

Honza
> 
> -- 
> Eric Botcazou

Re: [patch] Restore cross-language inlining into Ada

[Eric Botcazou]

> Hmm, I see now.  I wonder if we can also inline
> can_thorw_non_call_exceptions to !can_throw_non_call_exceptions provied
> that we set the flag in ipa-inline-transform.  That way we can inline Ada to
> C and the observation about no EH regions should still hold.

I'd say you're the only one caring about inlining Ada into other languages ;-)

-- 
Eric Botcazou

Re: [patch] Restore cross-language inlining into Ada

[Arnaud Charlet]

> > Hmm, I see now.  I wonder if we can also inline
> > can_thorw_non_call_exceptions to !can_throw_non_call_exceptions
> > provied
> > that we set the flag in ipa-inline-transform.  That way we can inline Ada to
> > C and the observation about no EH regions should still hold.
> 
> I'd say you're the only one caring about inlining Ada into other languages
> ;-)

Why do you say so? There are C->Ada calls as there are Ada->C calls in
plenty of existing software.

Arno

Re: [patch] Restore cross-language inlining into Ada

[Eric Botcazou]

> Why do you say so? There are C->Ada calls as there are Ada->C calls in
> plenty of existing software.

But what percentage of the C->Ada ones are performance critical?  Note that, 
unlike the Ada->C or Ada/C++ ones, these have never been inlined and I can 
imagine the kind of trouble this would introduce.  If the sofware contains a 
mix of C and Ada and some C->Ada calls are performance critical, then they'd 
better be rewritten in C, they will be optimized at any optimization level 
instead of just with LTO.

-- 
Eric Botcazou

Re: [patch] Restore cross-language inlining into Ada

[Jan Hubicka]

> > Why do you say so? There are C->Ada calls as there are Ada->C calls in
> > plenty of existing software.
> 
> But what percentage of the C->Ada ones are performance critical?  Note that, 
> unlike the Ada->C or Ada/C++ ones, these have never been inlined and I can 

I think we was inlining them with LTO until I installed the patch.  Most of time
DECL_STRUCT_FUNCTION == NULL for WPA and thus the original check testing the
flags was disabled.  We did not update the EH coddegen during inlining, so probably
we just did not produce non-call EH for these.

Honza

Re: [patch] Restore cross-language inlining into Ada

[Eric Botcazou]

> I think we was inlining them with LTO until I installed the patch.  Most of
> time DECL_STRUCT_FUNCTION == NULL for WPA and thus the original check
> testing the flags was disabled.  We did not update the EH coddegen during
> inlining, so probably we just did not produce non-call EH for these.

OK, we may have inlined them after all...  My understanding of the new code is 
that we will still inline them if the Ada callee doesn't use EH, which is good 
enough in my opinion.

-- 
Eric Botcazou

Re: [patch] Restore cross-language inlining into Ada

[Arnaud Charlet]

> OK, we may have inlined them after all...  My understanding of the new code is
> that we will still inline them if the Ada callee doesn't use EH, which is good
> enough in my opinion.

Agreed.

Re: [patch] Restore cross-language inlining into Ada

[Duncan Sands]

Hi Eric,

On 23/01/16 10:25, Eric Botcazou wrote:
>> I think we was inlining them with LTO until I installed the patch.  Most of
>> time DECL_STRUCT_FUNCTION == NULL for WPA and thus the original check
>> testing the flags was disabled.  We did not update the EH coddegen during
>> inlining, so probably we just did not produce non-call EH for these.
>
> OK, we may have inlined them after all...  My understanding of the new code is
> that we will still inline them if the Ada callee doesn't use EH, which is good
> enough in my opinion.

it would be nice to also inline if the caller doesn't use EH even if the callee 
does, for example when calling Ada from C.

Best wishes, Duncan.

Re: [patch] Restore cross-language inlining into Ada

[Richard Biener]

On Fri, Jan 22, 2016 at 1:00 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > I only updated
>> > -  /* 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.  */ -  else if (callee_fun &&
>> > callee_fun->can_throw_non_call_exceptions -    && !(caller_fun &&
>> > caller_fun->can_throw_non_call_exceptions)) -    {
>> > -      e->inline_failed = CIF_NON_CALL_EXCEPTIONS;
>> > -      inlinable = false;
>> > -    }
>> > to actually work with LTO where callee_fun/caller_fun is not always
>> > available (but sometimes, like when ICF requested the body or when we
>> > merged profiles, it is).
>>
>> No, that's not true.  Let's consider an Ada caller and a C callee.  With the
>> old code (mine as you remarked): caller_fun->can_throw_non_call_exceptions is
>> true and callee_fun->can_throw_non_call_exceptions is false, so the above test
>> is false and we can inline.  With the new code (yours): check_match is true
>> and opt_for_fn (callee->decl, flag_non_call_exceptions) is false, so we cannot
>> inline.
>
> Hmm, I see now.  I wonder if we can also inline can_thorw_non_call_exceptions
> to !can_throw_non_call_exceptions provied that we set the flag in
> ipa-inline-transform.  That way we can inline Ada to C and the observation
> about no EH regions should still hold.

That might work (same for -fexceptions).  You might want to wrap the function
in a ERT_MUST_NOT_THROW though in that case.

Richard.

> Honza
>>
>> --
>> Eric Botcazou