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 >> >