| Message ID | alpine.LSU.2.11.1611151556300.5294@t29.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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 > >
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
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.
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
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); +}