From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24364 invoked by alias); 1 Jun 2011 13:02:29 -0000 Received: (qmail 24353 invoked by uid 22791); 1 Jun 2011 13:02:27 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from eu1sys200aog106.obsmtp.com (HELO eu1sys200aog106.obsmtp.com) (207.126.144.121) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 01 Jun 2011 13:02:12 +0000 Received: from beta.dmz-eu.st.com ([164.129.1.35]) (using TLSv1) by eu1sys200aob106.postini.com ([207.126.147.11]) with SMTP ID DSNKTeY4UXvAtsUuaXrT7u10seEaRnjQxGGF@postini.com; Wed, 01 Jun 2011 13:02:11 UTC Received: from zeta.dmz-eu.st.com (ns2.st.com [164.129.230.9]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 39AC61A2; Wed, 1 Jun 2011 13:02:00 +0000 (GMT) Received: from Webmail-eu.st.com (safex1hubcas2.st.com [10.75.90.16]) by zeta.dmz-eu.st.com (STMicroelectronics) with ESMTP id E04B92AB9; Wed, 1 Jun 2011 13:01:59 +0000 (GMT) Received: from [164.129.122.89] (164.129.122.89) by webmail-eu.st.com (10.75.90.13) with Microsoft SMTP Server (TLS) id 8.2.234.1; Wed, 1 Jun 2011 15:01:59 +0200 Message-ID: <4DE63846.7060405@st.com> Date: Wed, 01 Jun 2011 13:02:00 -0000 From: Christian Bruel User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.17) Gecko/20110414 Thunderbird/3.1.10 ThunderBrowse/3.3.5 MIME-Version: 1.0 To: Richard Guenther Cc: GCC Patches , Jan Hubicka Subject: Re: [PATH] PR/49139 fix always_inline failures diagnostics References: <4DE49EBE.30804@st.com> <4DE5115B.4070409@st.com> In-Reply-To: Content-Type: multipart/mixed; boundary="------------080107070604080808080801" X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-06/txt/msg00027.txt.bz2 --------------080107070604080808080801 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Content-length: 8022 On 06/01/2011 12:02 PM, Richard Guenther wrote: > On Tue, May 31, 2011 at 6:03 PM, Christian Bruel wrote: >> >> >> On 05/31/2011 11:18 AM, Richard Guenther wrote: >>> >>> On Tue, May 31, 2011 at 9:54 AM, Christian Bruel >>> wrote: >>>> >>>> Hello, >>>> >>>> The attached patch fixes a few diagnostic discrepancies for always_inline >>>> failures. >>>> >>>> Illustrated by the fail_always_inline[12].c attached cases, the current >>>> behavior is one of: >>>> >>>> - success (with and without -Winline), silently not honoring >>>> always_inline >>>> gcc fail_always_inline1.c -S -Winline -O0 -fpic >>>> gcc fail_always_inline1.c -S -O2 -fpic >>>> >>>> - error: with -Winline but not without >>>> gcc fail_always_inline1.c -S -Winline -O2 -fpic >>>> >>>> - error: without -Winline >>>> gcc fail_always_inline2.c -S -fno-early-inlining -O2 >>>> or the original c++ attachment in this defect >>>> >>>> note that -Winline never warns, as stated in the documentation >>>> >>>> This simple patch consistently emits a warning (changing the sorry >>>> unimplemented message) whenever the attribute is not honored. >>>> >>>> My first will was to generate and error instead of the warning, but since >>>> it >>>> is possible that inlines is only performed at LTO time, an error would be >>>> inapropriate (Note that today this is not possible with -Winline that >>>> would >>>> abort). >>>> >>>> Another alternative I considered would be to emit the warning under >>>> -Winline >>>> rather than unconditionally, but this more a user misuse of the >>>> attribute, >>>> so should always be warned anyway. Or maybe a new -Winline-always that >>>> would >>>> be activated under -Wall ? Other opinion welcomed. >>>> >>>> Tested with standard bootstrap and regression on x86. >>>> >>>> Comments, and/or OK for trunk ? >>> >>> The patch is not ok, we may not fail to inline an always_inline >>> function. >> >> OK, I thought so that would be an error. but I was afraid to abort the >> inline of function with a missing body (provided by another file) by LTO, >> which would be a regression. rethinking about this and considering that a >> valid LTO program should be valid without LTO, and that the scope is the >> translation unit, that would be OK to always reject attribute_inline on >> functions without a body. >> >> To make this more consistent I proposed to warn >>> >>> whenever you take the address of an always_inline function >>> (because then you can confuse GCC by indirectly calling >>> such function which we might inline dependent on optimization >>> setting and which we might discover we didn't inline only >>> dependent on optimization setting).Honza proposed to move >>> the sorry()ing to when we feel the need to output the >>> always_inline function, thus when it was not optimized away, >>> but that would require us not preserving the body (do we?) >>> with -fpreserve-inline-functions. >>> >> >> But we don't know if taking the address of the function will result by a >> call to it, or how the pointer will be used. So I think the check should be >> done at the caller site. But I code like: >> >> inline __attribute__((always_inline)) int foo() { return 0; } >> >> static int (*ptr)() = foo; >> >> int >> bar() >> { >> return ptr(); >> } >> >> is not be (legitimately) inlined, but without diagnostic, I don't know where >> to look at this that at the moment. > > Yeah, the issue is that we only warn if we end up seeing a direct > call to an always_inline function that wasn't inlined. The idea of > sorrying() for remaining always_inline bodies instead would also > catch the above, but so would > > inline __attribute__((always_inline)) int foo() { return 0; } > int (*ptr)() = foo; > > (address-taken but not called). > >>> For fail_always_inline1.c we should diagnose the appearant >>> misuse of always_inline with a warning, drop the attribute >>> but keep DECL_DISREGARD_INLINE_LIMITS set. >>> >>> Same for fail_always_inline2.c. >>> >>> I agree that sorry()ing for those cases is odd. EIther we >>> should reject the declarations upfront >>> ("always-inline function will not be inlinable"), or we should >>> emit a warning of that kind and make sure to not sorry later. >> >> In addition to the error at the caller site, I've added the specific warn >> about the missing inline keyword. > > But not in a good place. Please instead check it alongside the > other attribute checks in cgraphunit.c:process_function_and_variable_attributes OK, the only difference is that we don't have the node analyzed here, so externally_visible, etc are not set. With the initial proposal the warning was emitted only if the function could not be inlined. Now it will be emitted for each DECL_COMDAT (decl) && !DECL_DECLARED_INLINE_P (decl)) even if not preemptible, so conservatively we don't want to duplicate the availability check. see attached new patch for that. > >> Thanks for your comments, here is the new patch that I'm testing, (without >> the handling of indirect calls which can be treated separately). > > Index: gcc/ipa-inline-transform.c > =================================================================== > --- gcc/ipa-inline-transform.c (revision 174264) > +++ gcc/ipa-inline-transform.c (working copy) > @@ -302,9 +302,20 @@ > > for (e = node->callees; e; e = e->next_callee) > { > - cgraph_redirect_edge_call_stmt_to_callee (e); > + gimple call = cgraph_redirect_edge_call_stmt_to_callee (e); > + > + if (!inline_p) > + { > if (!e->inline_failed || warn_inline) > inline_p = true; > + else > + { > + tree fn = gimple_call_fndecl (call); > + > + if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (fn))) > + inline_p = true; > + } > + } > } > > if (inline_p) > > Honza - this conditional calling of optimize_inline_calls just if > warn_inline is on is extra ugly. Does it really save that much > time to only conditionally run optimize_inline_calls? If so > we should re-write that function completely. > I fully agree, and this avoids the double check for the inline_attribute. However one alternative could be to promote the error checking from expand_call_inline in inline_transform only in Winline or always_inline. > Christian, for now I suggest to instead do > > Index: ipa-inline-transform.c > =================================================================== > --- ipa-inline-transform.c (revision 174520) > +++ ipa-inline-transform.c (working copy) > @@ -288,7 +288,6 @@ inline_transform (struct cgraph_node *no > { > unsigned int todo = 0; > struct cgraph_edge *e; > - bool inline_p = false; > > /* FIXME: Currently the pass manager is adding inline transform more than > once to some clones. This needs revisiting after WPA cleanups. */ > @@ -301,18 +300,12 @@ inline_transform (struct cgraph_node *no > save_inline_function_body (node); > > for (e = node->callees; e; e = e->next_callee) > - { > - cgraph_redirect_edge_call_stmt_to_callee (e); > - if (!e->inline_failed || warn_inline) > - inline_p = true; > - } > + cgraph_redirect_edge_call_stmt_to_callee (e); > + > + timevar_push (TV_INTEGRATION); > + todo = optimize_inline_calls (current_function_decl); > + timevar_pop (TV_INTEGRATION); > > - if (inline_p) > - { > - timevar_push (TV_INTEGRATION); > - todo = optimize_inline_calls (current_function_decl); > - timevar_pop (TV_INTEGRATION); > - } > cfun->always_inline_functions_inlined = true; > cfun->after_inlining = true; > return todo | execute_fixup_cfg (); > > this also looks like a recently introduced regression. > > I'm not sure about changing sorry () to error () (not even sure why > it was sorry in the first place ...). Any opinion from others? given that the sorry was emitted under -Winline, that was even more confusing. > > Thanks, > Richard. > >> Cheers >> >> Christian >> > --------------080107070604080808080801 Content-Type: text/plain; name="always_inline_warn.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="always_inline_warn.patch" Content-length: 4617 Index: gcc/cgraph.c =================================================================== Index: gcc/ipa-inline-transform.c =================================================================== --- gcc/ipa-inline-transform.c (revision 174264) +++ gcc/ipa-inline-transform.c (working copy) @@ -288,7 +288,6 @@ { unsigned int todo = 0; struct cgraph_edge *e; - bool inline_p = false; /* FIXME: Currently the pass manager is adding inline transform more than once to some clones. This needs revisiting after WPA cleanups. */ @@ -301,18 +300,12 @@ save_inline_function_body (node); for (e = node->callees; e; e = e->next_callee) - { cgraph_redirect_edge_call_stmt_to_callee (e); - if (!e->inline_failed || warn_inline) - inline_p = true; - } - if (inline_p) - { timevar_push (TV_INTEGRATION); todo = optimize_inline_calls (current_function_decl); timevar_pop (TV_INTEGRATION); - } + cfun->always_inline_functions_inlined = true; cfun->after_inlining = true; return todo | execute_fixup_cfg (); Index: gcc/cgraphunit.c =================================================================== --- gcc/cgraphunit.c (revision 174264) +++ gcc/cgraphunit.c (working copy) @@ -900,6 +900,13 @@ DECL_ATTRIBUTES (decl) = remove_attribute ("weakref", DECL_ATTRIBUTES (decl)); } + if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl))) + { + if (! DECL_COMDAT (decl) && !DECL_DECLARED_INLINE_P (decl)) + warning (OPT_Wattributes, + "always_inline function might not be inlinable"); + } + process_common_attributes (decl); } for (vnode = varpool_nodes; vnode != first_var; vnode = vnode->next) Index: gcc/testsuite/gcc.dg/always_inline.c =================================================================== --- gcc/testsuite/gcc.dg/always_inline.c (revision 174264) +++ gcc/testsuite/gcc.dg/always_inline.c (working copy) @@ -1,8 +1,7 @@ /* { dg-do compile } */ -/* { dg-options "-Winline -O2" } */ #include inline __attribute__ ((always_inline)) void -e(int t, ...) /* { dg-message "sorry\[^\n\]*variable argument" "" } */ +e(int t, ...) /* { dg-error "variable argument" } */ { va_list q; va_start (q, t); Index: gcc/testsuite/gcc.dg/always_inline2.c =================================================================== --- gcc/testsuite/gcc.dg/always_inline2.c (revision 174264) +++ gcc/testsuite/gcc.dg/always_inline2.c (working copy) @@ -1,8 +1,7 @@ /* { dg-do compile } */ -/* { dg-options "-Winline -O2" } */ -inline __attribute__ ((always_inline)) void t(void); /* { dg-message "sorry\[^\n\]*body not available" "" } */ +inline __attribute__ ((always_inline)) void t(void); /* { dg-error "body not available" } */ void q(void) { - t(); /* { dg-message "sorry\[^\n\]*called from here" "" } */ + t(); /* { dg-error "called from here" } */ } Index: gcc/testsuite/gcc.dg/always_inline3.c =================================================================== --- gcc/testsuite/gcc.dg/always_inline3.c (revision 174264) +++ gcc/testsuite/gcc.dg/always_inline3.c (working copy) @@ -1,11 +1,11 @@ /* { dg-do compile } */ -/* { dg-options "-Winline -O2" } */ +/* { dg-options "-O2" } */ int do_something_evil (void); inline __attribute__ ((always_inline)) void -q2(void) /* { dg-message "sorry\[^\n\]*recursive" "" } */ +q2(void) /* { dg-error "recursive inlining" } */ { if (do_something_evil ()) return; - q2(); /* { dg-message "sorry\[^\n\]*called from here" "" } */ + q2(); /* { dg-error "called from here" } */ q2(); /* With -O2 we don't warn here, it is eliminated by tail recursion. */ } Index: gcc/tree-inline.c =================================================================== --- gcc/tree-inline.c (revision 174264) +++ gcc/tree-inline.c (working copy) @@ -3192,7 +3192,7 @@ As a bonus we can now give more details about the reason why a function is not inlinable. */ if (always_inline) - sorry (inline_forbidden_reason, fn); + error (inline_forbidden_reason, fn); else if (do_warning) warning (OPT_Winline, inline_forbidden_reason, fn); @@ -3744,9 +3744,9 @@ /* Avoid warnings during early inline pass. */ && cgraph_global_info_ready) { - sorry ("inlining failed in call to %q+F: %s", fn, - _(cgraph_inline_failed_string (reason))); - sorry ("called from here"); + error ("inlining failed in call to always_inline %q+F: %s", fn, + cgraph_inline_failed_string (reason)); + error ("called from here"); } else if (warn_inline && DECL_DECLARED_INLINE_P (fn) --------------080107070604080808080801--