| Message ID | CAAs8HmyuOuM_HkKoJ+s0LBb7946Db5M9wqAmnct=DL-O4eV2xg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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. > > > > Thanks > Sri > > On Mon, Jun 10, 2013 at 4:10 PM, Sriraman Tallam <tmsriram@google.com> wrote: >> Ping. >> >> On Tue, Jun 4, 2013 at 2:41 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>> Ping. >>> >>> On Thu, May 23, 2013 at 2:41 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>>> Ping, for review of ipa-inline.c change. >>>> >>>> Sri >>>> >>>> On Mon, May 20, 2013 at 11:04 AM, Sriraman Tallam <tmsriram@google.com> wrote: >>>>> On Fri, May 17, 2013 at 11:21 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>> On Fri, May 17, 2013 at 09:00:21PM -0700, Sriraman Tallam wrote: >>>>>>> --- ipa-inline.c (revision 198950) >>>>>>> +++ ipa-inline.c (working copy) >>>>>>> @@ -374,7 +374,33 @@ can_early_inline_edge_p (struct cgraph_edge *e) >>>>>>> return false; >>>>>>> } >>>>>>> if (!can_inline_edge_p (e, true)) >>>>>>> - return false; >>>>>>> + { >>>>>>> + enum availability avail; >>>>>>> + struct cgraph_node *callee >>>>>>> + = cgraph_function_or_thunk_node (e->callee, &avail); >>>>>>> + /* Flag an error when the inlining cannot happen because of target option >>>>>>> + mismatch but the callee is marked as "always_inline". In -O0 mode >>>>>>> + this will go undetected because the error flagged in >>>>>>> + "expand_call_inline" in tree-inline.c might not execute and the >>>>>>> + inlining will not happen. Then, the linker could complain about a >>>>>>> + missing body for the callee if it turned out that the callee was >>>>>>> + also marked "gnu_inline" with extern inline keyword as bodies of such >>>>>>> + functions are not generated. */ >>>>>>> + if ((!optimize >>>>>>> + || flag_no_inline) >>>>>> >>>>>> This should be if ((!optimize || flag_no_inline) on one line. >>>>>> >>>>>> I'd prefer also the testcase for the ICEs, something like: >>>>>> >>>>>> /* Test case to check if AVX intrinsics and function specific target >>>>>> optimizations work together. Check by including x86intrin.h */ >>>>>> >>>>>> /* { dg-do compile } */ >>>>>> /* { dg-options "-O2 -mno-sse -mno-avx" } */ >>>>>> >>>>>> #include <x86intrin.h> >>>>>> >>>>>> __m256 a, b, c; >>>>>> void __attribute__((target ("avx"))) >>>>>> foo (void) >>>>>> { >>>>>> a = _mm256_and_ps (b, c); >>>>>> } >>>>>> >>>>>> and another testcase that does: >>>>>> >>>>>> /* { dg-do compile } */ >>>>>> #pragma GCC target ("mavx") /* { dg-error "whatever" } */ >>>>>> >>>>>> Otherwise it looks good to me, but I'd prefer the i?86 maintainers to review >>>>>> it too (and Honza for ipa-inline.c?). >>>>> >>>>> Honza, could you please take a look at the ipa-inline.c fix? I will >>>>> split the patches and submit after Honza's review. I will also make >>>>> the changes mentioned. >>>>> >>>>> Thanks >>>>> Sri >>>>> >>>>> >>>>>> >>>>>> Jakub
> 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. Honza
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 + ...)? Thanks, Sri > > Honza
> 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? Honza
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 extern gnu_inline .* target specific option mismatch" } */ +{ + return 0; +} + +int bar() +{ + return foo (); +} /* { dg-error "called from here" } */ Index: ipa-inline.c =================================================================== --- ipa-inline.c (revision 200034) +++ ipa-inline.c (working copy) @@ -374,7 +374,31 @@ can_early_inline_edge_p (struct cgraph_edge *e) return false; } if (!can_inline_edge_p (e, true)) - return false; + { + enum availability avail; + struct cgraph_node *callee + = cgraph_function_or_thunk_node (e->callee, &avail); + /* Flag an error here when the inlining cannot happen because of target + option mismatch but the callee is marked as "always_inline". + Otherwise, in -O0 mode this could go unreported because the error + flagged in "expand_call_inline" in tree-inline.c might not execute. + Then, the linker could complain about a missing body for the callee + if it turned out that the callee was also marked "gnu_inline" with + extern inline keyword as bodies of such functions are not + generated. */ + if (!optimize + && e->inline_failed == CIF_TARGET_OPTION_MISMATCH + && lookup_attribute ("always_inline", DECL_ATTRIBUTES (callee->symbol.decl)) + && lookup_attribute ("gnu_inline", DECL_ATTRIBUTES (callee->symbol.decl)) + && DECL_DECLARED_INLINE_P (callee->symbol.decl)) + { + error ("inlining failed in call to extern gnu_inline %q+F: %s", + callee->symbol.decl, + cgraph_inline_failed_string (CIF_TARGET_OPTION_MISMATCH)); + error ("called from here"); + } + return false; + } return true; }
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. Thanks Sri On Mon, Jun 10, 2013 at 4:10 PM, Sriraman Tallam <tmsriram@google.com> wrote: > Ping. > > On Tue, Jun 4, 2013 at 2:41 PM, Sriraman Tallam <tmsriram@google.com> wrote: >> Ping. >> >> On Thu, May 23, 2013 at 2:41 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>> Ping, for review of ipa-inline.c change. >>> >>> Sri >>> >>> On Mon, May 20, 2013 at 11:04 AM, Sriraman Tallam <tmsriram@google.com> wrote: >>>> On Fri, May 17, 2013 at 11:21 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>> On Fri, May 17, 2013 at 09:00:21PM -0700, Sriraman Tallam wrote: >>>>>> --- ipa-inline.c (revision 198950) >>>>>> +++ ipa-inline.c (working copy) >>>>>> @@ -374,7 +374,33 @@ can_early_inline_edge_p (struct cgraph_edge *e) >>>>>> return false; >>>>>> } >>>>>> if (!can_inline_edge_p (e, true)) >>>>>> - return false; >>>>>> + { >>>>>> + enum availability avail; >>>>>> + struct cgraph_node *callee >>>>>> + = cgraph_function_or_thunk_node (e->callee, &avail); >>>>>> + /* Flag an error when the inlining cannot happen because of target option >>>>>> + mismatch but the callee is marked as "always_inline". In -O0 mode >>>>>> + this will go undetected because the error flagged in >>>>>> + "expand_call_inline" in tree-inline.c might not execute and the >>>>>> + inlining will not happen. Then, the linker could complain about a >>>>>> + missing body for the callee if it turned out that the callee was >>>>>> + also marked "gnu_inline" with extern inline keyword as bodies of such >>>>>> + functions are not generated. */ >>>>>> + if ((!optimize >>>>>> + || flag_no_inline) >>>>> >>>>> This should be if ((!optimize || flag_no_inline) on one line. >>>>> >>>>> I'd prefer also the testcase for the ICEs, something like: >>>>> >>>>> /* Test case to check if AVX intrinsics and function specific target >>>>> optimizations work together. Check by including x86intrin.h */ >>>>> >>>>> /* { dg-do compile } */ >>>>> /* { dg-options "-O2 -mno-sse -mno-avx" } */ >>>>> >>>>> #include <x86intrin.h> >>>>> >>>>> __m256 a, b, c; >>>>> void __attribute__((target ("avx"))) >>>>> foo (void) >>>>> { >>>>> a = _mm256_and_ps (b, c); >>>>> } >>>>> >>>>> and another testcase that does: >>>>> >>>>> /* { dg-do compile } */ >>>>> #pragma GCC target ("mavx") /* { dg-error "whatever" } */ >>>>> >>>>> Otherwise it looks good to me, but I'd prefer the i?86 maintainers to review >>>>> it too (and Honza for ipa-inline.c?). >>>> >>>> Honza, could you please take a look at the ipa-inline.c fix? I will >>>> split the patches and submit after Honza's review. I will also make >>>> the changes mentioned. >>>> >>>> Thanks >>>> Sri >>>> >>>> >>>>> >>>>> Jakub * 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.