[PATH] PR/49139 fix always_inline failures diagnostics

[Richard Guenther]

On Wed, Jun 1, 2011 at 3:01 PM, Christian Bruel <christian.bruel@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.bruel@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.bruel@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
>>>
>>
>