Patchwork GCC does not support *mmintrin.h with function specific opts

login
register
mail settings
Submitter Sriraman Tallam
Date June 13, 2013, 1 a.m.
Message ID <CAAs8HmyuOuM_HkKoJ+s0LBb7946Db5M9wqAmnct=DL-O4eV2xg@mail.gmail.com>
Download mbox | patch
Permalink /patch/250949/
State New
Headers show

Comments

Sriraman Tallam - June 13, 2013, 1 a.m.
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.
Xinliang David Li - June 13, 2013, 5:01 p.m.
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
Jan Hubicka - June 13, 2013, 5:07 p.m.
> 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
Sriraman Tallam - June 13, 2013, 5:11 p.m.
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
Jan Hubicka - June 13, 2013, 5:19 p.m.
> 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

Patch

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;
 }