On Wed, Jun 1, 2011 at 3:01 PM, Christian Bruel <christian.br...@st.com> wrote:
>
>
> On 06/01/2011 12:02 PM, Richard Guenther wrote:
>>
>> On Tue, May 31, 2011 at 6:03 PM, Christian Bruel<christian.br...@st.com>
>> wrote:
>>>
>>>
>>> On 05/31/2011 11:18 AM, Richard Guenther wrote:
>>>>
>>>> On Tue, May 31, 2011 at 9:54 AM, Christian Bruel<christian.br...@st.com>
>>>> 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.
Hm, I'm confused. Do all DECL_COMDAT functions have the
always_inline attribute set? I would have expected
+ if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl)))
+ {
+ if (!DECL_DECLARED_INLINE_P (decl))
+ warning (OPT_Wattributes,
+ "always_inline not declared inline might not be
inlinable");
+ }
do you get excessive warnings with this?
> 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.
Yeah, I can imagine that - I only noticed this when looking at your
patch context.
Thanks,
Richard
>>
>> Thanks,
>> Richard.
>>
>>> Cheers
>>>
>>> Christian
>>>
>>
>
