| Message ID | 9690839.xiTXAUyZ0b@polaris |
|---|---|
| State | New |
| Headers | show |
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
> 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
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
> > > > 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
> 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.
> 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.
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
> > 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
> 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 ;-)
> > 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
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
> 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.
> > 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
> 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.
> 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.
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.
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