[PATCH] Fix PR78306

[PATCH] Fix PR78306

[Richard Biener]

Appearantly for some unknown reason we refuse to inline anything into
functions calling cilk_spawn.  That breaks fortified headers and
all other always-inline function calls (intrinsics come to my mind as 
well).

Bootstrapped and tested on x86_64-unknown-linux-gnu, ok for trunk?

Thanks,
Richard.

2016-11-15  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/78306
	* ipa-inline-analysis.c (initialize_inline_failed): Do not
	inhibit inlining if function calls cilk_spawn.
	(can_inline_edge_p): Likewise.

	* gcc.dg/cilk-plus/pr78306.c: New testcase.

Index: gcc/ipa-inline-analysis.c
===================================================================
--- gcc/ipa-inline-analysis.c	(revision 242408)
+++ gcc/ipa-inline-analysis.c	(working copy)
@@ -1507,9 +1507,6 @@ initialize_inline_failed (struct cgraph_
     e->inline_failed = CIF_BODY_NOT_AVAILABLE;
   else if (callee->local.redefined_extern_inline)
     e->inline_failed = CIF_REDEFINED_EXTERN_INLINE;
-  else if (cfun && fn_contains_cilk_spawn_p (cfun))
-    /* We can't inline if the function is spawing a function.  */
-    e->inline_failed = CIF_CILK_SPAWN;
   else
     e->inline_failed = CIF_FUNCTION_NOT_CONSIDERED;
   gcc_checking_assert (!e->call_stmt_cannot_inline_p
Index: gcc/ipa-inline.c
===================================================================
--- gcc/ipa-inline.c	(revision 242408)
+++ gcc/ipa-inline.c	(working copy)
@@ -368,11 +368,6 @@ can_inline_edge_p (struct cgraph_edge *e
       e->inline_failed = CIF_FUNCTION_NOT_INLINABLE;
       inlinable = false;
     }
-  else if (inline_summaries->get (caller)->contains_cilk_spawn)
-    {
-      e->inline_failed = CIF_CILK_SPAWN;
-      inlinable = false;
-    }
   /* Don't inline a function with mismatched sanitization attributes. */
   else if (!sanitize_attrs_match_for_inline_p (caller->decl, callee->decl))
     {
Index: gcc/testsuite/gcc.dg/cilk-plus/pr78306.c
===================================================================
--- gcc/testsuite/gcc.dg/cilk-plus/pr78306.c	(revision 0)
+++ gcc/testsuite/gcc.dg/cilk-plus/pr78306.c	(working copy)
@@ -0,0 +1,30 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -fcilkplus" } */
+
+#define _FORTIFY_SOURCE=2
+#include <cilk/cilk.h>
+#include <string.h>
+#include <malloc.h>
+
+int sum(int low, int high)
+{
+  if(low == high) {
+    return low;
+  }
+
+  int mid = low + (high-low)/2;
+  int a = cilk_spawn sum(low, mid);
+  int b = sum(mid+1, high);
+
+  // Some very expensive computation here
+  int foo[64];
+  memset(foo, 0, 64*sizeof(int)); // <--- Fails
+
+  cilk_sync;
+
+  return a+b;
+}
+
+int main(void) {
+  return sum(0, 100);
+}

Re: [PATCH] Fix PR78306

[Richard Biener]

On Tue, 15 Nov 2016, Richard Biener wrote:

> 
> Appearantly for some unknown reason we refuse to inline anything into
> functions calling cilk_spawn.  That breaks fortified headers and
> all other always-inline function calls (intrinsics come to my mind as 
> well).
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu, ok for trunk?

I had to fix the testcase somewhat to avoid the cilk.h header.  I also
added correctness checking.

I'll commit this to trunk if there are no comments on why we refuse
to inline into cilk-spawn containing functions (CCing original
author as well) - the code behaves this way since the original
merge and there's no testcase showing failure when removing it.

Thanks,
Richard.

2016-11-16  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/78306
	* ipa-inline-analysis.c (initialize_inline_failed): Do not
	inhibit inlining if function calls cilk_spawn.
	(can_inline_edge_p): Likewise.

	* gcc.dg/cilk-plus/pr78306.c: New testcase.

Index: gcc/ipa-inline-analysis.c
===================================================================
--- gcc/ipa-inline-analysis.c	(revision 242408)
+++ gcc/ipa-inline-analysis.c	(working copy)
@@ -1507,9 +1507,6 @@ initialize_inline_failed (struct cgraph_
     e->inline_failed = CIF_BODY_NOT_AVAILABLE;
   else if (callee->local.redefined_extern_inline)
     e->inline_failed = CIF_REDEFINED_EXTERN_INLINE;
-  else if (cfun && fn_contains_cilk_spawn_p (cfun))
-    /* We can't inline if the function is spawing a function.  */
-    e->inline_failed = CIF_CILK_SPAWN;
   else
     e->inline_failed = CIF_FUNCTION_NOT_CONSIDERED;
   gcc_checking_assert (!e->call_stmt_cannot_inline_p
Index: gcc/ipa-inline.c
===================================================================
--- gcc/ipa-inline.c	(revision 242408)
+++ gcc/ipa-inline.c	(working copy)
@@ -368,11 +368,6 @@ can_inline_edge_p (struct cgraph_edge *e
       e->inline_failed = CIF_FUNCTION_NOT_INLINABLE;
       inlinable = false;
     }
-  else if (inline_summaries->get (caller)->contains_cilk_spawn)
-    {
-      e->inline_failed = CIF_CILK_SPAWN;
-      inlinable = false;
-    }
   /* Don't inline a function with mismatched sanitization attributes. */
   else if (!sanitize_attrs_match_for_inline_p (caller->decl, callee->decl))
     {
Index: gcc/testsuite/gcc.dg/cilk-plus/pr78306.c
===================================================================
--- gcc/testsuite/gcc.dg/cilk-plus/pr78306.c	(revision 0)
+++ gcc/testsuite/gcc.dg/cilk-plus/pr78306.c	(revision 0)
@@ -0,0 +1,30 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -fcilkplus" } */
+
+#define _FORTIFY_SOURCE 2
+#include <string.h>
+
+int sum(int low, int high)
+{
+  if(low == high) {
+    return low;
+  }
+
+  int mid = low + (high-low)/2;
+  int a = _Cilk_spawn sum(low, mid);
+  int b = sum(mid+1, high);
+
+  // Some very expensive computation here
+  int foo[64];
+  memset(foo, 0, 64*sizeof(int)); // <--- Fails
+
+  _Cilk_sync;
+
+  return a+b;
+}
+
+int main(void) {
+  if (sum(0, 100) != 5050)
+    __builtin_abort ();
+  return 0;
+}

Re: [PATCH] Fix PR78306

[Jeff Law]

On 11/15/2016 07:58 AM, Richard Biener wrote:
>
> Appearantly for some unknown reason we refuse to inline anything into
> functions calling cilk_spawn.  That breaks fortified headers and
> all other always-inline function calls (intrinsics come to my mind as
> well).
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, ok for trunk?
>
> Thanks,
> Richard.
>
> 2016-11-15  Richard Biener  <rguenther@suse.de>
>
> 	PR tree-optimization/78306
> 	* ipa-inline-analysis.c (initialize_inline_failed): Do not
> 	inhibit inlining if function calls cilk_spawn.
> 	(can_inline_edge_p): Likewise.
>
> 	* gcc.dg/cilk-plus/pr78306.c: New testcase.
Balaji added this check explicitly. There should be tests in the 
testsuite (spawnee_inline, spawner_inline) which exercise that code.

I suspect those bits were taken into the trunk as-is and were missed 
during the review cycle.  The actual tests look to only verify that we 
don't crash at runtime -- they do nothing to verify we get the correct 
results.

My understanding of Cilk is that it should be possible to literally 
define away the Cilk keywords and the resulting program should "just 
work".  It would seem that if we must avoid inlining to get proper 
behavior around a cilk_spawn then cilk_spawn is perhaps more fragile 
than it ought to be.


Jeff

Re: [PATCH] Fix PR78306

[Richard Biener]

On Mon, 28 Nov 2016, Jeff Law wrote:

> On 11/15/2016 07:58 AM, Richard Biener wrote:
> > 
> > Appearantly for some unknown reason we refuse to inline anything into
> > functions calling cilk_spawn.  That breaks fortified headers and
> > all other always-inline function calls (intrinsics come to my mind as
> > well).
> > 
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, ok for trunk?
> > 
> > Thanks,
> > Richard.
> > 
> > 2016-11-15  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR tree-optimization/78306
> > 	* ipa-inline-analysis.c (initialize_inline_failed): Do not
> > 	inhibit inlining if function calls cilk_spawn.
> > 	(can_inline_edge_p): Likewise.
> > 
> > 	* gcc.dg/cilk-plus/pr78306.c: New testcase.
> Balaji added this check explicitly. There should be tests in the testsuite
> (spawnee_inline, spawner_inline) which exercise that code.

Yes he did, but no, nothing in the testsuite.

> I suspect those bits were taken into the trunk as-is and were missed during
> the review cycle.  The actual tests look to only verify that we don't crash at
> runtime -- they do nothing to verify we get the correct results.

Well, I don't know Cilk+ but it seems to be unmaintained.  Maybe it's time
to rip it out.

> My understanding of Cilk is that it should be possible to literally define
> away the Cilk keywords and the resulting program should "just work".  It would
> seem that if we must avoid inlining to get proper behavior around a cilk_spawn
> then cilk_spawn is perhaps more fragile than it ought to be.

There is _nowhere_ documented _why_ the checks were added.  Why is 
inlining a transform that can do anything bad to a function using
cilk_spawn?

Note that we can't simply disable inlining here (see the PR).

So what exactly is it that we need to avoid?  Inlining of sth
calling cilk_spawn into a function calling cilk_spawn?

Note CCing several intel people didn't come up with any answer.  And
appearantly no, there was no testcase added for this issue.  It all
was present in the initial commit.

Richard.

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix PR78306

[Jeff Law]

On 11/29/2016 12:47 AM, Richard Biener wrote:
>> Balaji added this check explicitly. There should be tests in the testsuite
>> (spawnee_inline, spawner_inline) which exercise that code.
>
> Yes he did, but no, nothing in the testsuite.
I believe the tests are:

c-c++-common/cilk-plus/CK/spawnee_inline.c
c-c++-common/cilk-plus/CK/spawner_inline.c

But as I mentioned, they don't check for proper behaviour


>
> There is _nowhere_ documented _why_ the checks were added.  Why is
> inlining a transform that can do anything bad to a function using
> cilk_spawn?
I know, it's disappointing.  Even the tests mentioned above don't shed 
any real light on the issue.


Jeff

Re: [PATCH] Fix PR78306

[Richard Biener]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1817 bytes --]

On Tue, 29 Nov 2016, Jeff Law wrote:

> On 11/29/2016 12:47 AM, Richard Biener wrote:
> > > Balaji added this check explicitly. There should be tests in the testsuite
> > > (spawnee_inline, spawner_inline) which exercise that code.
> > 
> > Yes he did, but no, nothing in the testsuite.
> I believe the tests are:
> 
> c-c++-common/cilk-plus/CK/spawnee_inline.c
> c-c++-common/cilk-plus/CK/spawner_inline.c
> 
> But as I mentioned, they don't check for proper behaviour

Actually they do -- and both show what the issue might be, cilk+
uses setjmp but we already have code to disallow inlining of
functions calling setjmp (but we happily inline into functions
calling setjmp).  When mangling the testcases to try forcing
inlining I still (the patch was already applied) get

/space/rguenther/src/gcc-git/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c: 
In function ‘fib’:
/space/rguenther/src/gcc-git/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c:9:50: 
error: function ‘fib’ can never be copied because it receives a non-local 
goto

so the intent was probably to disallow inlining of functions calling
cilk_spawn, not to disable inlining into functions calling cilk_spawn.

But as seen above this is already handled by generic code handling
setjmp.

> 
> > 
> > There is _nowhere_ documented _why_ the checks were added.  Why is
> > inlining a transform that can do anything bad to a function using
> > cilk_spawn?
> I know, it's disappointing.  Even the tests mentioned above don't shed any
> real light on the issue.

One issue is obvious (but already handled).  Why all inlining should
be disabled is indeed still a mystery.

Richard.

> 
> Jeff
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix PR78306

[Andrew Senkevich]

2016-11-30 11:52 GMT+03:00 Richard Biener <rguenther@suse.de>:
> On Tue, 29 Nov 2016, Jeff Law wrote:
>
>> On 11/29/2016 12:47 AM, Richard Biener wrote:
>> > > Balaji added this check explicitly. There should be tests in the testsuite
>> > > (spawnee_inline, spawner_inline) which exercise that code.
>> >
>> > Yes he did, but no, nothing in the testsuite.
>> I believe the tests are:
>>
>> c-c++-common/cilk-plus/CK/spawnee_inline.c
>> c-c++-common/cilk-plus/CK/spawner_inline.c
>>
>> But as I mentioned, they don't check for proper behaviour
>
> Actually they do -- and both show what the issue might be, cilk+
> uses setjmp but we already have code to disallow inlining of
> functions calling setjmp (but we happily inline into functions
> calling setjmp).  When mangling the testcases to try forcing
> inlining I still (the patch was already applied) get
>
> /space/rguenther/src/gcc-git/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c:
> In function ‘fib’:
> /space/rguenther/src/gcc-git/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c:9:50:
> error: function ‘fib’ can never be copied because it receives a non-local
> goto
>
> so the intent was probably to disallow inlining of functions calling
> cilk_spawn, not to disable inlining into functions calling cilk_spawn.
>
> But as seen above this is already handled by generic code handling
> setjmp.
>
>>
>> >
>> > There is _nowhere_ documented _why_ the checks were added.  Why is
>> > inlining a transform that can do anything bad to a function using
>> > cilk_spawn?
>> I know, it's disappointing.  Even the tests mentioned above don't shed any
>> real light on the issue.
>
> One issue is obvious (but already handled).  Why all inlining should
> be disabled is indeed still a mystery.

I can suppose inline should be disabled for the next function after
cilk_spawn because spawn should be done for function.
If no way to disable the next call inlining it looks it was disabled
for all function to fix Cilk Plus Conformance Suite test fail.


--
WBR,
Andrew

Re: [PATCH] Fix PR78306

[Richard Biener]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6218 bytes --]

On Wed, 30 Nov 2016, Andrew Senkevich wrote:

> 2016-11-30 11:52 GMT+03:00 Richard Biener <rguenther@suse.de>:
> > On Tue, 29 Nov 2016, Jeff Law wrote:
> >
> >> On 11/29/2016 12:47 AM, Richard Biener wrote:
> >> > > Balaji added this check explicitly. There should be tests in the testsuite
> >> > > (spawnee_inline, spawner_inline) which exercise that code.
> >> >
> >> > Yes he did, but no, nothing in the testsuite.
> >> I believe the tests are:
> >>
> >> c-c++-common/cilk-plus/CK/spawnee_inline.c
> >> c-c++-common/cilk-plus/CK/spawner_inline.c
> >>
> >> But as I mentioned, they don't check for proper behaviour
> >
> > Actually they do -- and both show what the issue might be, cilk+
> > uses setjmp but we already have code to disallow inlining of
> > functions calling setjmp (but we happily inline into functions
> > calling setjmp).  When mangling the testcases to try forcing
> > inlining I still (the patch was already applied) get
> >
> > /space/rguenther/src/gcc-git/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c:
> > In function Β‘fibΒ’:
> > /space/rguenther/src/gcc-git/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c:9:50:
> > error: function Β‘fibΒ’ can never be copied because it receives a non-local
> > goto
> >
> > so the intent was probably to disallow inlining of functions calling
> > cilk_spawn, not to disable inlining into functions calling cilk_spawn.
> >
> > But as seen above this is already handled by generic code handling
> > setjmp.
> >
> >>
> >> >
> >> > There is _nowhere_ documented _why_ the checks were added.  Why is
> >> > inlining a transform that can do anything bad to a function using
> >> > cilk_spawn?
> >> I know, it's disappointing.  Even the tests mentioned above don't shed any
> >> real light on the issue.
> >
> > One issue is obvious (but already handled).  Why all inlining should
> > be disabled is indeed still a mystery.
> 
> I can suppose inline should be disabled for the next function after
> cilk_spawn because spawn should be done for function.
> If no way to disable the next call inlining it looks it was disabled
> for all function to fix Cilk Plus Conformance Suite test fail.

I see.  Even with GCC 6.2 the conformace test suite has a lot of FAILs
(and compiler ICEs):

In 734 tests: 229 (60 compilation, 169 execution) tests do not conform 
specification, 505 conform

using the suite in version 1.2.1.  For current trunk I get

In 734 tests: 198 (43 compilation, 155 execution) tests do not conform 
specification, 536 conform

where gcc 6.2 vs. gcc 7 diff is

-  PASS cn_cilk_for_label_in.c
-  PASS cn_cilk_for_label_out.c
+  FAIL cn_cilk_for_label_in.c (internal compiler error)
+  FAIL cn_cilk_for_label_out.c (internal compiler error)
-  PASS cn_cilk_for_return.c
+  FAIL cn_cilk_for_return.c (internal compiler error)
-  PASS ep_cilk_for_compare1.c
+  FAIL ep_cilk_for_compare1.c (internal compiler error)
-  PASS ep_cilk_for_increment1.c
+  FAIL ep_cilk_for_increment1.c (internal compiler error)
-  PASS ep_cilk_for_nest1.c
-  PASS ep_cilk_for_pragma2.c
+  FAIL ep_cilk_for_nest1.c (internal compiler error)
+  FAIL ep_cilk_for_pragma2.c (internal compiler error)
-  PASS cn_cilk_for_init1.cpp
+  FAIL cn_cilk_for_init1.cpp (internal compiler error)
-  FAIL cn_omp_simd_lastprivate2-1.c (internal compiler error)
-  FAIL cn_omp_simd_lastprivate2-2.c (internal compiler error)
-  FAIL cn_omp_simd_lastprivate3.c
-  FAIL cn_omp_simd_lastprivate4-1.c (internal compiler error)
+  PASS cn_omp_simd_lastprivate2-1.c
+  PASS cn_omp_simd_lastprivate2-2.c
+  PASS cn_omp_simd_lastprivate3.c
+  PASS cn_omp_simd_lastprivate4-1.c
-  FAIL cn_omp_simd_linear2.c
-  FAIL cn_omp_simd_linear3-1.c (internal compiler error)
+  PASS cn_omp_simd_linear2.c
+  PASS cn_omp_simd_linear3-1.c
-  FAIL cn_omp_simd_linear6-2.c
+  PASS cn_omp_simd_linear6-2.c
-  FAIL cn_omp_simd_private3.c
-  FAIL cn_omp_simd_private4-1.c (internal compiler error)
+  PASS cn_omp_simd_private3.c
+  PASS cn_omp_simd_private4-1.c
-  FAIL cn_omp_simd_reduction1.c (internal compiler error)
-  FAIL cn_omp_simd_reduction2.c (internal compiler error)
-  FAIL cn_omp_simd_reduction3.c
-  FAIL cn_omp_simd_reduction4-1.c
-  FAIL cn_omp_simd_reduction4-2.c (internal compiler error)
-  FAIL cn_omp_simd_reduction5.c (internal compiler error)
+  PASS cn_omp_simd_reduction1.c
+  PASS cn_omp_simd_reduction2.c
+  PASS cn_omp_simd_reduction3.c
+  PASS cn_omp_simd_reduction4-1.c
+  PASS cn_omp_simd_reduction4-2.c
+  FAIL cn_omp_simd_reduction5.c
-  FAIL cn_simd_linear1.c
-  FAIL cn_simd_linear2.c
-  FAIL cn_simd_linear3.c
+  PASS cn_simd_linear1.c
+  PASS cn_simd_linear2.c
+  PASS cn_simd_linear3.c
-  FAIL cn_omp_simd_lastprivate1.cpp
-  FAIL cn_omp_simd_lastprivate2.cpp
+  PASS cn_omp_simd_lastprivate1.cpp
+  PASS cn_omp_simd_lastprivate2.cpp
-  FAIL cn_omp_simd_linear2-1.cpp
+  PASS cn_omp_simd_linear2-1.cpp
-  FAIL cn_omp_simd_private1.cpp
-  FAIL cn_omp_simd_private2.cpp
+  PASS cn_omp_simd_private1.cpp
+  PASS cn_omp_simd_private2.cpp
-  FAIL ep_omp_simd_reduction_bool_10.cpp (internal compiler error)
+  PASS ep_omp_simd_reduction_bool_10.cpp
-  FAIL ep_omp_simd_reduction_bool_9.cpp (internal compiler error)
+  PASS ep_omp_simd_reduction_bool_9.cpp
-  FAIL ep_omp_simd_reduction_char_10.cpp (internal compiler error)
+  PASS ep_omp_simd_reduction_char_10.cpp
...
-  PASS cp_an_section_triplet.c
+  FAIL cp_an_section_triplet.c (internal compiler error)
-  FAIL ep_an_op_reduce_fn4.cpp
-  FAIL ep_an_op_reduce_fn5.cpp
+  FAIL ep_an_op_reduce_fn4.cpp (internal compiler error)
+  FAIL ep_an_op_reduce_fn5.cpp (internal compiler error)
-  FAIL ep_an_op_reduce_fn7.cpp
+  FAIL ep_an_op_reduce_fn7.cpp (internal compiler error)
-  FAIL ep_an_hyperobject_reducer_list.cpp (internal compiler error)
+  PASS ep_an_hyperobject_reducer_list.cpp
...

so progess on the OMP front hides regressions elsewhere.  The patch
that started this thread does not have any effect on the conformance
testsuite result.

That makes it even more obvious that it is a) unmaintained, b) probably
not used widely as those ICEs have not been reported as bugs

Let's deprecate Cilk+ if no maintainer steps up until 7.1 is released.  
Gerald, can you relay that to the SC?

Richard.

Re: [PATCH] Fix PR78306

[Jeff Law]

On 11/30/2016 07:18 AM, Richard Biener wrote:

>
> so progess on the OMP front hides regressions elsewhere.  The patch
> that started this thread does not have any effect on the conformance
> testsuite result.
>
> That makes it even more obvious that it is a) unmaintained, b) probably
> not used widely as those ICEs have not been reported as bugs
>
> Let's deprecate Cilk+ if no maintainer steps up until 7.1 is released.
> Gerald, can you relay that to the SC?
I've raised the issue with the steering committee; I will also speak 
with Intel about the possibility of them providing resources to maintain 
this code.

jeff