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

login
register
mail settings
Submitter Sriraman Tallam
Date June 13, 2013, 6:33 p.m.
Message ID <CAAs8Hmz6Lav0DRx8Aw8TQ4Qm4=06Ok5ArL_h+YKZQMCk1hRLRw@mail.gmail.com>
Download mbox | patch
Permalink /patch/251147/
State New
Headers show

Comments

Sriraman Tallam - June 13, 2013, 6:33 p.m.
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.
Xinliang David Li - June 13, 2013, 7:25 p.m.
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
Jan Hubicka - June 13, 2013, 7:40 p.m.
> 	* 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
Jan Hubicka - June 13, 2013, 7:45 p.m.
> 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
Xinliang David Li - June 13, 2013, 7:59 p.m.
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
Sriraman Tallam - June 14, 2013, 2:52 a.m.
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
Richard Guenther - June 14, 2013, 8:43 a.m.
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
Jan Hubicka - June 14, 2013, 8:46 a.m.
> 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
Jakub Jelinek - June 14, 2013, 8:49 a.m.
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

Patch

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" } */
+}