From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29737 invoked by alias); 1 Jun 2011 10:03:09 -0000 Received: (qmail 29727 invoked by uid 22791); 1 Jun 2011 10:03:07 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RFC_ABUSE_POST X-Spam-Check-By: sourceware.org Received: from mail-wy0-f175.google.com (HELO mail-wy0-f175.google.com) (74.125.82.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 01 Jun 2011 10:02:52 +0000 Received: by wye20 with SMTP id 20so4674789wye.20 for ; Wed, 01 Jun 2011 03:02:51 -0700 (PDT) MIME-Version: 1.0 Received: by 10.227.152.132 with SMTP id g4mr6987912wbw.24.1306922570908; Wed, 01 Jun 2011 03:02:50 -0700 (PDT) Received: by 10.227.37.152 with HTTP; Wed, 1 Jun 2011 03:02:50 -0700 (PDT) In-Reply-To: <4DE5115B.4070409@st.com> References: <4DE49EBE.30804@st.com> <4DE5115B.4070409@st.com> Date: Wed, 01 Jun 2011 10:03:00 -0000 Message-ID: Subject: Re: [PATH] PR/49139 fix always_inline failures diagnostics From: Richard Guenther To: Christian Bruel Cc: GCC Patches , Jan Hubicka Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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/msg00022.txt.bz2 On Tue, May 31, 2011 at 6:03 PM, Christian Bruel w= rote: > > > On 05/31/2011 11:18 AM, Richard Guenther wrote: >> >> On Tue, May 31, 2011 at 9:54 AM, Christian Bruel >> =A0wrote: >>> >>> Hello, >>> >>> The attached patch fixes a few diagnostic discrepancies for always_inli= ne >>> 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 >>> =A0 gcc fail_always_inline1.c -S -Winline -O0 -fpic >>> =A0 gcc fail_always_inline1.c -S -O2 -fpic >>> >>> - error: with -Winline but not without >>> =A0 gcc fail_always_inline1.c -S -Winline -O2 -fpic >>> >>> - error: without -Winline >>> =A0 gcc fail_always_inline2.c -S -fno-early-inlining -O2 >>> =A0 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 sin= ce >>> 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)) =A0int foo() { return 0; } > > static int (*ptr)() =3D foo; > > int > bar() > { > =A0return ptr(); > } > > is not be (legitimately) inlined, but without diagnostic, I don't know wh= ere > 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)() =3D 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. =A0EIther 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_attrib= utes > 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- gcc/ipa-inline-transform.c (revision 174264) +++ gcc/ipa-inline-transform.c (working copy) @@ -302,9 +302,20 @@ for (e =3D node->callees; e; e =3D e->next_callee) { - cgraph_redirect_edge_call_stmt_to_callee (e); + gimple call =3D cgraph_redirect_edge_call_stmt_to_callee (e); + + if (!inline_p) + { if (!e->inline_failed || warn_inline) inline_p =3D true; + else + { + tree fn =3D gimple_call_fndecl (call); +=09 + if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (fn))) + inline_p =3D 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. Christian, for now I suggest to instead do Index: ipa-inline-transform.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- 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 =3D 0; struct cgraph_edge *e; - bool inline_p =3D 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 =3D node->callees; e; e =3D e->next_callee) - { - cgraph_redirect_edge_call_stmt_to_callee (e); - if (!e->inline_failed || warn_inline) - inline_p =3D true; - } + cgraph_redirect_edge_call_stmt_to_callee (e); + + timevar_push (TV_INTEGRATION); + todo =3D optimize_inline_calls (current_function_decl); + timevar_pop (TV_INTEGRATION); - if (inline_p) - { - timevar_push (TV_INTEGRATION); - todo =3D optimize_inline_calls (current_function_decl); - timevar_pop (TV_INTEGRATION); - } cfun->always_inline_functions_inlined =3D true; cfun->after_inlining =3D 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? Thanks, Richard. > Cheers > > Christian >