| Message ID | CAAs8Hmz6Lav0DRx8Aw8TQ4Qm4=06Ok5ArL_h+YKZQMCk1hRLRw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
If you want to flag errors for all possible wrongly used always_inline attribute, should this change be done in can_inline_edge_p? Or keep your current change, but also add a warning (something like 'always inline function is ignored etc') in inline_always_inline_functions when inline transformation can not meaningfully give a useful error. David On Thu, Jun 13, 2013 at 11:33 AM, Sriraman Tallam <tmsriram@google.com> wrote: > On Thu, Jun 13, 2013 at 10:19 AM, Jan Hubicka <hubicka@ucw.cz> wrote: >>> On Thu, Jun 13, 2013 at 10:07 AM, Jan Hubicka <hubicka@ucw.cz> wrote: >>> >> Can you create a helper function to flag the error and perhaps also >>> >> put that check inside can_inline_edge_p ? >>> >> >>> >> David >>> >> >>> >> >>> >> On Wed, Jun 12, 2013 at 6:00 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>> >> > Hi Honza, >>> >> > >>> >> > I have isolated the ipa-inline.c part into a separate patch with a >>> >> > test and attached it here. The patch is simple. Could you please take >>> >> > a look? >>> >> > >>> >> > * ipa-inline.c (can_early_inline_edge_p): Flag an error when >>> >> > the function that cannot be inlined is target specific. >>> >> > * gcc.target/i386/inline_error.c: New test. >>> > >>> > Sorry for taking ages to look at the patch, I was too hooked into other problems. >>> > I also think can_early_inline_edge_p should not produce diagnostic - it is supposed >>> > to be predicate. >>> > >>> > So your problem is that the hard worker in tree-inline is not called at -O0 >>> > and thus errors are not output? I would suggest arranging inline_always_inline_functions >>> > to return true even if inlining failed and thus making inline_calls to be called. >>> >>> Thanks Honza! Yes, that is the problem. Should I just make it return >>> true for these special conditions (TARGET_MISMATCH + gnu_inline + >>> always_inline + ...)? >> >> Can't it just return true if there is any alwaysinline call? I think if inline fails, >> we always want to error, right? > > Ok, patch attached that does this. Please let me know what you think. > > Thanks > Sri > >> >> Honza
> * tree-inline.c (expand_call_inline): Allow the error to be flagged > in early inline pass. > * ipa-inline.c (inline_always_inline_functions): Pretend always_inline > functions are inlined during failures to flag an error. > * gcc.target/i386/inline_error.c: New test. This patch is OK if it passes testing. Thanks for your patience! Honza
> If you want to flag errors for all possible wrongly used always_inline > attribute, should this change be done in can_inline_edge_p? Or keep > your current change, but also add a warning (something like 'always > inline function is ignored etc') in inline_always_inline_functions > when inline transformation can not meaningfully give a useful error. We have the CIF error codes to be able to give useful diagnostic at transformation time. I think it is better to have all the diagnostic output at one place unless we have really good reasons to fork it. We are not losing any precision here, right? Sure, other option would be to move all alwaysinline diagnostic into inline_always_inline_functions and remove the code path in tree-inline. The warnings however are quite meaningfully places in tree-inline, because we want to warn only after all the inlining algorithms has finished and inlining really did not happen. They can be moved out there if we walk the edges at end and output diagnostic, but I do not see anything really wrong with their current location. Honza
yes, what you said makes sense. thanks, David On Thu, Jun 13, 2013 at 12:45 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> If you want to flag errors for all possible wrongly used always_inline >> attribute, should this change be done in can_inline_edge_p? Or keep >> your current change, but also add a warning (something like 'always >> inline function is ignored etc') in inline_always_inline_functions >> when inline transformation can not meaningfully give a useful error. > > We have the CIF error codes to be able to give useful diagnostic at > transformation time. I think it is better to have all the diagnostic output at > one place unless we have really good reasons to fork it. We are not losing any > precision here, right? > > Sure, other option would be to move all alwaysinline diagnostic into > inline_always_inline_functions and remove the code path in tree-inline. The > warnings however are quite meaningfully places in tree-inline, because we want > to warn only after all the inlining algorithms has finished and inlining really > did not happen. They can be moved out there if we walk the edges at end and > output diagnostic, but I do not see anything really wrong with their current > location. > > Honza
On Thu, Jun 13, 2013 at 12:40 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> * tree-inline.c (expand_call_inline): Allow the error to be flagged >> in early inline pass. >> * ipa-inline.c (inline_always_inline_functions): Pretend always_inline >> functions are inlined during failures to flag an error. >> * gcc.target/i386/inline_error.c: New test. > This patch is OK if it passes testing. Two tests gcc.c-torture/compile/pr43791.c and pr44043.c are failing because of always_inline functions being present that cannot be inlined and the compiler is now generating error messages. I will fix them and resend the patch. Thanks Sri > Thanks for your patience! > > Honza
On Fri, Jun 14, 2013 at 4:52 AM, Sriraman Tallam <tmsriram@google.com> wrote: > On Thu, Jun 13, 2013 at 12:40 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >>> * tree-inline.c (expand_call_inline): Allow the error to be flagged >>> in early inline pass. >>> * ipa-inline.c (inline_always_inline_functions): Pretend always_inline >>> functions are inlined during failures to flag an error. >>> * gcc.target/i386/inline_error.c: New test. > >> This patch is OK if it passes testing. > > Two tests gcc.c-torture/compile/pr43791.c and pr44043.c are failing > because of always_inline functions being present that cannot be > inlined and the compiler is now generating error messages. I will fix > them and resend the patch. Quick look - pr43791.c is not expected to work at -O0, so skip -O0 for example by guarding the whole thing with #if __OPTIMIZED__ > 0. Similar for pr44043.c. That we didn't error at -O0 before is a bug. Eventually I was suggesting to error if we end up outputting the body of an always_inline function, that is, any uses remain (including indirect calls or address-takens which is where we do not error right now). Richard. > Thanks > Sri > >> Thanks for your patience! > > > >> >> Honza
> On Fri, Jun 14, 2013 at 4:52 AM, Sriraman Tallam <tmsriram@google.com> wrote: > > On Thu, Jun 13, 2013 at 12:40 PM, Jan Hubicka <hubicka@ucw.cz> wrote: > >>> * tree-inline.c (expand_call_inline): Allow the error to be flagged > >>> in early inline pass. > >>> * ipa-inline.c (inline_always_inline_functions): Pretend always_inline > >>> functions are inlined during failures to flag an error. > >>> * gcc.target/i386/inline_error.c: New test. > > > >> This patch is OK if it passes testing. > > > > Two tests gcc.c-torture/compile/pr43791.c and pr44043.c are failing > > because of always_inline functions being present that cannot be > > inlined and the compiler is now generating error messages. I will fix > > them and resend the patch. > > Quick look - pr43791.c is not expected to work at -O0, so skip -O0 > for example by guarding the whole thing with #if __OPTIMIZED__ > 0. > Similar for pr44043.c. > > That we didn't error at -O0 before is a bug. Eventually I was suggesting > to error if we end up outputting the body of an always_inline function, > that is, any uses remain (including indirect calls or address-takens which > is where we do not error right now). Well, as dicussed earlier, this will make kernel folks crazy, since they define inline to be always inline and likes to take address of that function. (of course that is crazy already) Honza > > Richard. > > > Thanks > > Sri > > > >> Thanks for your patience! > > > > > > > >> > >> Honza
On Fri, Jun 14, 2013 at 10:43:34AM +0200, Richard Biener wrote: > On Fri, Jun 14, 2013 at 4:52 AM, Sriraman Tallam <tmsriram@google.com> wrote: > > On Thu, Jun 13, 2013 at 12:40 PM, Jan Hubicka <hubicka@ucw.cz> wrote: > >>> * tree-inline.c (expand_call_inline): Allow the error to be flagged > >>> in early inline pass. > >>> * ipa-inline.c (inline_always_inline_functions): Pretend always_inline > >>> functions are inlined during failures to flag an error. > >>> * gcc.target/i386/inline_error.c: New test. > > > >> This patch is OK if it passes testing. > > > > Two tests gcc.c-torture/compile/pr43791.c and pr44043.c are failing > > because of always_inline functions being present that cannot be > > inlined and the compiler is now generating error messages. I will fix > > them and resend the patch. > > Quick look - pr43791.c is not expected to work at -O0, so skip -O0 > for example by guarding the whole thing with #if __OPTIMIZED__ > 0. > Similar for pr44043.c. > > That we didn't error at -O0 before is a bug. Eventually I was suggesting > to error if we end up outputting the body of an always_inline function, > that is, any uses remain (including indirect calls or address-takens which > is where we do not error right now). Well, for indirect calls/address-takens and gnu_inline style extern inline always_inline, it should be fine if we just emit calls to the library function, otherwise you'd break a lot of code, because glibc uses always_inline heavily for -D_FORTIFY_SOURCE wrapper inlines, open argument checking etc. But that body supposedly doesn't inherit the always_inline attribute. Jakub
Index: tree-inline.c =================================================================== --- tree-inline.c (revision 200034) +++ tree-inline.c (working copy) @@ -3905,8 +3905,6 @@ expand_call_inline (basic_block bb, gimple stmt, c for inlining, but we can't do that because frontends overwrite the body. */ && !cg_edge->callee->local.redefined_extern_inline - /* Avoid warnings during early inline pass. */ - && cgraph_global_info_ready /* PR 20090218-1_0.c. Body can be provided by another module. */ && (reason != CIF_BODY_NOT_AVAILABLE || !flag_generate_lto)) { Index: ipa-inline.c =================================================================== --- ipa-inline.c (revision 200034) +++ ipa-inline.c (working copy) @@ -1911,7 +1911,15 @@ inline_always_inline_functions (struct cgraph_node } if (!can_early_inline_edge_p (e)) - continue; + { + /* Set inlined to true if the callee is marked "always_inline" but + is not inlinable. This will allow flagging an error later in + expand_call_inline in tree-inline.c. */ + if (lookup_attribute ("always_inline", + DECL_ATTRIBUTES (callee->symbol.decl)) != NULL) + inlined = true; + continue; + } if (dump_file) fprintf (dump_file, " Inlining %s into %s (always_inline).\n", Index: testsuite/gcc.target/i386/inline_error.c =================================================================== --- testsuite/gcc.target/i386/inline_error.c (revision 0) +++ testsuite/gcc.target/i386/inline_error.c (revision 0) @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-O0 -mno-popcnt" } */ + +inline int __attribute__ ((__gnu_inline__, __always_inline__, target("popcnt"))) +foo () /* { dg-error "inlining failed in call to always_inline .* target specific option mismatch" } */ +{ + return 0; +} + +int bar() +{ + return foo (); /* { dg-error "called from here" } */ +}