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.
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
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 <stdarg.h>
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)
