| Message ID | BF230D13CA30DD48930C31D4099330003B0F002A@fmsmsx101.amr.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
On 08/31/2015 06:04 PM, Iyer, Balaji V wrote: > Hello Everyone, > This patch will fix the bug reported in Bugzilla, PR 60586. The issue was that the spawned function's function arguments must not be pushed into the nested/lambda function. This patch should fix that issue. > > I have tested this on x86_64 (linux and Cygwin flavors). Is this OK for trunk? > > Here are the Changelog entries: > > gcc/c-family/ChangeLog > 2015-08-31 Balaji V. Iyer<balaji.v.iyer@intel.com> > > PR middle-end/60586 > * c-common.h: Added two more parameters to the gimplify_cilk_spawn > function. > * c-gimplify.c (c_gimplify_expr): Likewise. > * cilk.c (gimplify_cilk_spawn): Likewise and called > gimplify_call_params_in_spawned_fn. > (gimplify_call_params_in_spawned_fn): New function. > > gcc/cp/ChangeLog > 2015-08-31 Balaji V. Iyer<balaji.v.iyer@intel.com> > > PR middle-end/60586 > * cp-gimplify.c (cp_gimplify_expr): Added two additional parameters to > gimplify_cilk_spawn. > > gcc/testsuite/ChangeLog > 2015-08-31 Balaji V. Iyer<balaji.v.iyer@intel.com> > > PR middle-end/60586 > * c-c++-common/cilk-plus/CK/pr60586.c: New file. > * g++.dg/cilk-plus/CK/pr60586.cc: Likewise. > > > Thanks, > > Balaji V. Iyer. > > > diff.txt > > > diff --git a/gcc/c-family/cilk.c b/gcc/c-family/cilk.c > index 1012a4f..1fe6685 100644 > --- a/gcc/c-family/cilk.c > +++ b/gcc/c-family/cilk.c > @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see > #include "gimplify.h" > #include "tree-iterator.h" > #include "tree-inline.h" > +#include "cp/cp-tree.h" Presumably you needed this for AGGR_INIT_EXPR. I believe you need to refactor the code a bit -- including a C++ front-end header file into something in c-family seems wrong. While it may not break in this specific instance, but it is a modularity violation. > #include "c-family/c-common.h" > #include "toplev.h" > #include "tm.h" > @@ -762,12 +763,37 @@ create_cilk_wrapper (tree exp, tree *args_out) > return fndecl; > } > > +/* Gimplify all the parameters for the Spawned function. *EXPR_P can be a > + CALL_EXPR, INIT_EXPR, MODIFY_EXPR or TARGET_EXPR. *PRE_P and *POST_P are > + gimple sequences from the caller of gimplify_cilk_spawn. */ Comment doesn't match the code, code also checks for AGGR_INIT_EXPR. Given the dependency on AGGR_INIT_EXPR, it seems this checking somehow needs to move into the front-ends. I think once that's fixed this ought to be ready for the trunk. jeff
> -----Original Message----- > From: Jeff Law [mailto:law@redhat.com] > Sent: Tuesday, September 1, 2015 3:26 PM > To: Iyer, Balaji V; gcc-patches@gcc.gnu.org > Cc: Zamyatin, Igor > Subject: Re: [PATCH] PR 60586 > > On 08/31/2015 06:04 PM, Iyer, Balaji V wrote: > > Hello Everyone, > > This patch will fix the bug reported in Bugzilla, PR 60586. The issue > was that the spawned function's function arguments must not be pushed > into the nested/lambda function. This patch should fix that issue. > > > > I have tested this on x86_64 (linux and Cygwin flavors). Is this OK for trunk? > > > > Here are the Changelog entries: > > > > gcc/c-family/ChangeLog > > 2015-08-31 Balaji V. Iyer<balaji.v.iyer@intel.com> > > > > PR middle-end/60586 > > * c-common.h: Added two more parameters to the > gimplify_cilk_spawn > > function. > > * c-gimplify.c (c_gimplify_expr): Likewise. > > * cilk.c (gimplify_cilk_spawn): Likewise and called > > gimplify_call_params_in_spawned_fn. > > (gimplify_call_params_in_spawned_fn): New function. > > > > gcc/cp/ChangeLog > > 2015-08-31 Balaji V. Iyer<balaji.v.iyer@intel.com> > > > > PR middle-end/60586 > > * cp-gimplify.c (cp_gimplify_expr): Added two additional parameters > to > > gimplify_cilk_spawn. > > > > gcc/testsuite/ChangeLog > > 2015-08-31 Balaji V. Iyer<balaji.v.iyer@intel.com> > > > > PR middle-end/60586 > > * c-c++-common/cilk-plus/CK/pr60586.c: New file. > > * g++.dg/cilk-plus/CK/pr60586.cc: Likewise. > > > > > > Thanks, > > > > Balaji V. Iyer. > > > > > > diff.txt > > > > > > diff --git a/gcc/c-family/cilk.c b/gcc/c-family/cilk.c index > > 1012a4f..1fe6685 100644 > > --- a/gcc/c-family/cilk.c > > +++ b/gcc/c-family/cilk.c > > @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see > > #include "gimplify.h" > > #include "tree-iterator.h" > > #include "tree-inline.h" > > +#include "cp/cp-tree.h" > Presumably you needed this for AGGR_INIT_EXPR. I believe you need to > refactor the code a bit -- including a C++ front-end header file into something > in c-family seems wrong. > > While it may not break in this specific instance, but it is a modularity violation. > > > > > #include "c-family/c-common.h" > > #include "toplev.h" > > #include "tm.h" > > @@ -762,12 +763,37 @@ create_cilk_wrapper (tree exp, tree *args_out) > > return fndecl; > > } > > > > +/* Gimplify all the parameters for the Spawned function. *EXPR_P can be > a > > + CALL_EXPR, INIT_EXPR, MODIFY_EXPR or TARGET_EXPR. *PRE_P and > *POST_P are > > + gimple sequences from the caller of gimplify_cilk_spawn. */ > Comment doesn't match the code, code also checks for AGGR_INIT_EXPR. > > Given the dependency on AGGR_INIT_EXPR, it seems this checking > somehow > needs to move into the front-ends. Will you be OK if I make the function "gimplify_call_params_in_spawned_fn" part of the targetm structure? Thanks, Balaji V. Iyer. > > I think once that's fixed this ought to be ready for the trunk. > > jeff
> -----Original Message----- > From: Iyer, Balaji V > Sent: Tuesday, September 1, 2015 6:17 PM > To: 'Jeff Law'; gcc-patches@gcc.gnu.org > Cc: Zamyatin, Igor > Subject: RE: [PATCH] PR 60586 > > > > > -----Original Message----- > > From: Jeff Law [mailto:law@redhat.com] > > Sent: Tuesday, September 1, 2015 3:26 PM > > To: Iyer, Balaji V; gcc-patches@gcc.gnu.org > > Cc: Zamyatin, Igor > > Subject: Re: [PATCH] PR 60586 > > > > On 08/31/2015 06:04 PM, Iyer, Balaji V wrote: > > > Hello Everyone, > > > This patch will fix the bug reported in Bugzilla, PR 60586. The > > > issue > > was that the spawned function's function arguments must not be pushed > > into the nested/lambda function. This patch should fix that issue. > > > > > > I have tested this on x86_64 (linux and Cygwin flavors). Is this OK for > trunk? > > > > > > Here are the Changelog entries: > > > > > > gcc/c-family/ChangeLog > > > 2015-08-31 Balaji V. Iyer<balaji.v.iyer@intel.com> > > > > > > PR middle-end/60586 > > > * c-common.h: Added two more parameters to the > > gimplify_cilk_spawn > > > function. > > > * c-gimplify.c (c_gimplify_expr): Likewise. > > > * cilk.c (gimplify_cilk_spawn): Likewise and called > > > gimplify_call_params_in_spawned_fn. > > > (gimplify_call_params_in_spawned_fn): New function. > > > > > > gcc/cp/ChangeLog > > > 2015-08-31 Balaji V. Iyer<balaji.v.iyer@intel.com> > > > > > > PR middle-end/60586 > > > * cp-gimplify.c (cp_gimplify_expr): Added two additional > > > parameters > > to > > > gimplify_cilk_spawn. > > > > > > gcc/testsuite/ChangeLog > > > 2015-08-31 Balaji V. Iyer<balaji.v.iyer@intel.com> > > > > > > PR middle-end/60586 > > > * c-c++-common/cilk-plus/CK/pr60586.c: New file. > > > * g++.dg/cilk-plus/CK/pr60586.cc: Likewise. > > > > > > > > > Thanks, > > > > > > Balaji V. Iyer. > > > > > > > > > diff.txt > > > > > > > > > diff --git a/gcc/c-family/cilk.c b/gcc/c-family/cilk.c index > > > 1012a4f..1fe6685 100644 > > > --- a/gcc/c-family/cilk.c > > > +++ b/gcc/c-family/cilk.c > > > @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see > > > #include "gimplify.h" > > > #include "tree-iterator.h" > > > #include "tree-inline.h" > > > +#include "cp/cp-tree.h" > > Presumably you needed this for AGGR_INIT_EXPR. I believe you need to > > refactor the code a bit -- including a C++ front-end header file into > > something in c-family seems wrong. > > > > While it may not break in this specific instance, but it is a modularity > violation. > > > > > > > > > #include "c-family/c-common.h" > > > #include "toplev.h" > > > #include "tm.h" > > > @@ -762,12 +763,37 @@ create_cilk_wrapper (tree exp, tree *args_out) > > > return fndecl; > > > } > > > > > > +/* Gimplify all the parameters for the Spawned function. *EXPR_P > > > +can be > > a > > > + CALL_EXPR, INIT_EXPR, MODIFY_EXPR or TARGET_EXPR. *PRE_P and > > *POST_P are > > > + gimple sequences from the caller of gimplify_cilk_spawn. */ > > Comment doesn't match the code, code also checks for AGGR_INIT_EXPR. > > > > Given the dependency on AGGR_INIT_EXPR, it seems this checking > somehow > > needs to move into the front-ends. > > Will you be OK if I make the function "gimplify_call_params_in_spawned_fn" > part of the targetm structure? > Sorry, I meant lang_hooks. > Thanks, > > Balaji V. Iyer. > > > > > I think once that's fixed this ought to be ready for the trunk. > > > > jeff
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index be63cd2..027bea6 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1423,7 +1423,7 @@ extern vec <tree, va_gc> *fix_sec_implicit_args /* In cilk.c. */ extern tree insert_cilk_frame (tree); extern void cilk_init_builtins (void); -extern int gimplify_cilk_spawn (tree *); +extern int gimplify_cilk_spawn (tree *, gimple_seq *, gimple_seq *); extern void cilk_install_body_with_frame_cleanup (tree, tree, void *); extern bool cilk_detect_spawn_and_unwrap (tree *); extern bool cilk_set_spawn_marker (location_t, tree); diff --git a/gcc/c-family/c-gimplify.c b/gcc/c-family/c-gimplify.c index 1c81773..fd954d3 100644 --- a/gcc/c-family/c-gimplify.c +++ b/gcc/c-family/c-gimplify.c @@ -288,7 +288,8 @@ c_gimplify_expr (tree *expr_p, gimple_seq *pre_p ATTRIBUTE_UNUSED, /* If errors are seen, then just process it as a CALL_EXPR. */ if (!seen_error ()) - return (enum gimplify_status) gimplify_cilk_spawn (expr_p); + return (enum gimplify_status) gimplify_cilk_spawn (expr_p, pre_p, + post_p); case MODIFY_EXPR: case INIT_EXPR: @@ -299,7 +300,8 @@ c_gimplify_expr (tree *expr_p, gimple_seq *pre_p ATTRIBUTE_UNUSED, original expression (MODIFY/INIT/CALL_EXPR) is processes as it is supposed to be. */ && !seen_error ()) - return (enum gimplify_status) gimplify_cilk_spawn (expr_p); + return (enum gimplify_status) gimplify_cilk_spawn (expr_p, pre_p, + post_p); default:; } diff --git a/gcc/c-family/cilk.c b/gcc/c-family/cilk.c index 1012a4f..1fe6685 100644 --- a/gcc/c-family/cilk.c +++ b/gcc/c-family/cilk.c @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see #include "gimplify.h" #include "tree-iterator.h" #include "tree-inline.h" +#include "cp/cp-tree.h" #include "c-family/c-common.h" #include "toplev.h" #include "tm.h" @@ -762,12 +763,37 @@ create_cilk_wrapper (tree exp, tree *args_out) return fndecl; } +/* Gimplify all the parameters for the Spawned function. *EXPR_P can be a + CALL_EXPR, INIT_EXPR, MODIFY_EXPR or TARGET_EXPR. *PRE_P and *POST_P are + gimple sequences from the caller of gimplify_cilk_spawn. */ + +static void +gimplify_call_params_in_spawned_fn (tree *expr_p, gimple_seq *pre_p, + gimple_seq *post_p) +{ + int ii = 0; + tree *fix_parm_expr = expr_p; + if ((TREE_CODE (*expr_p) == INIT_EXPR) + || (TREE_CODE (*expr_p) == TARGET_EXPR) + || (TREE_CODE (*expr_p) == MODIFY_EXPR)) + fix_parm_expr = &TREE_OPERAND (*expr_p, 1); + + if (TREE_CODE (*fix_parm_expr) == CALL_EXPR) + for (ii = 0; ii < call_expr_nargs (*fix_parm_expr); ii++) + gimplify_expr (&CALL_EXPR_ARG (*fix_parm_expr, ii), pre_p, post_p, + is_gimple_reg, fb_rvalue); + else if (TREE_CODE (*fix_parm_expr) == AGGR_INIT_EXPR) + for (ii = 0; ii < aggr_init_expr_nargs (*fix_parm_expr); ii++) + gimplify_expr (&AGGR_INIT_EXPR_ARG (*fix_parm_expr, ii), pre_p, post_p, + is_gimple_reg, fb_rvalue); +} + /* Transform *SPAWN_P, a spawned CALL_EXPR, to gimple. *SPAWN_P can be a CALL_EXPR, INIT_EXPR or MODIFY_EXPR. Returns GS_OK if everything is fine, and GS_UNHANDLED, otherwise. */ int -gimplify_cilk_spawn (tree *spawn_p) +gimplify_cilk_spawn (tree *spawn_p, gimple_seq *pre_p, gimple_seq *post_p) { tree expr = *spawn_p; tree function, call1, call2, new_args; @@ -784,6 +810,8 @@ gimplify_cilk_spawn (tree *spawn_p) while (TREE_CODE (expr) == CLEANUP_POINT_EXPR || TREE_CODE (expr) == EXPR_STMT) expr = TREE_OPERAND (expr, 0); + + gimplify_call_params_in_spawned_fn (&expr, pre_p, post_p); new_args = NULL; function = create_cilk_wrapper (expr, &new_args); diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c index c36d339..8aacbbb 100644 --- a/gcc/cp/cp-gimplify.c +++ b/gcc/cp/cp-gimplify.c @@ -603,7 +603,8 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p) if (fn_contains_cilk_spawn_p (cfun) && cilk_detect_spawn_and_unwrap (expr_p) && !seen_error ()) - return (enum gimplify_status) gimplify_cilk_spawn (expr_p); + return (enum gimplify_status) gimplify_cilk_spawn (expr_p, pre_p, + post_p); cp_gimplify_init_expr (expr_p); if (TREE_CODE (*expr_p) != INIT_EXPR) return GS_OK; @@ -614,7 +615,8 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p) if (fn_contains_cilk_spawn_p (cfun) && cilk_detect_spawn_and_unwrap (expr_p) && !seen_error ()) - return (enum gimplify_status) gimplify_cilk_spawn (expr_p); + return (enum gimplify_status) gimplify_cilk_spawn (expr_p, pre_p, + post_p); /* If the back end isn't clever enough to know that the lhs and rhs types are the same, add an explicit conversion. */ @@ -715,13 +717,15 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p) /* If errors are seen, then just process it as a CALL_EXPR. */ if (!seen_error ()) - return (enum gimplify_status) gimplify_cilk_spawn (expr_p); + return (enum gimplify_status) gimplify_cilk_spawn (expr_p, pre_p, + post_p); case CALL_EXPR: if (fn_contains_cilk_spawn_p (cfun) && cilk_detect_spawn_and_unwrap (expr_p) && !seen_error ()) - return (enum gimplify_status) gimplify_cilk_spawn (expr_p); + return (enum gimplify_status) gimplify_cilk_spawn (expr_p, pre_p, + post_p); /* DR 1030 says that we need to evaluate the elements of an initializer-list in forward order even when it's used as arguments to diff --git a/gcc/testsuite/c-c++-common/cilk-plus/CK/pr60586.c b/gcc/testsuite/c-c++-common/cilk-plus/CK/pr60586.c new file mode 100644 index 0000000..c4012a0 --- /dev/null +++ b/gcc/testsuite/c-c++-common/cilk-plus/CK/pr60586.c @@ -0,0 +1,28 @@ +/* { dg-do run { target { i?86-*-* x86_64-*-* } } } */ +/* { dg-options "-fcilkplus -O2" } */ +/* { dg-additional-options "-lcilkrts" { target { i?86-*-* x86_64-*-* } } } */ + +int noop(int x) +{ + return x; +} + +int post_increment(int *x) +{ + return (*x)++; +} + +int main(int argc, char *argv[]) +{ + int m = 5; + int n = m; + int r = _Cilk_spawn noop(post_increment(&n)); + int n2 = n; + _Cilk_sync; + + if (r != m || n2 != m + 1) + return 1; + else + return 0; +} + diff --git a/gcc/testsuite/g++.dg/cilk-plus/CK/pr60586.cc b/gcc/testsuite/g++.dg/cilk-plus/CK/pr60586.cc new file mode 100644 index 0000000..6a27cad --- /dev/null +++ b/gcc/testsuite/g++.dg/cilk-plus/CK/pr60586.cc @@ -0,0 +1,89 @@ +/* { dg-options "-fcilkplus" } */ +/* { dg-do run { target i?86-*-* x86_64-*-* arm*-*-* } } */ +/* { dg-options "-fcilkplus -lcilkrts" { target { i?86-*-* x86_64-*-* } } } */ + +class Rectangle +{ + int area_val, h, w; + public: + Rectangle (int, int); + Rectangle (int, int, int); + ~Rectangle (); + int area (); +}; +Rectangle::~Rectangle () +{ + h = 0; + w = 0; + area_val = 0; +} +Rectangle::Rectangle (int height, int width) +{ + h = height; + w = width; + area_val = 0; +} + +int some_func(int &x) +{ + x++; + return x; +} + +Rectangle::Rectangle (int height, int width, int area_orig) +{ + h = height; + w = width; + area_val = area_orig; +} + +int Rectangle::area() +{ + return (area_val += (h*w)); +} + + +int some_func (int &); + +/* Spawning constructor. */ +int main1 (void) +{ + int x = 3; + Rectangle r = _Cilk_spawn Rectangle (some_func(x), 3); + return r.area(); +} + +/* Spawning constructor 2. */ +int main2 (void) +{ + Rectangle r (_Cilk_spawn Rectangle (4, 3)); + return r.area(); +} + +/* Spawning copy constructor. */ +int main3 (void) +{ + int x = 3; + Rectangle r = _Cilk_spawn Rectangle (some_func(x), 3, 2); + return r.area (); +} + +/* Spawning copy constructor 2. */ +int main4 (void) +{ + Rectangle r ( _Cilk_spawn Rectangle (4, 3, 2)); + return r.area(); +} + +int main (void) +{ + if (main1 () != 12) + __builtin_abort (); + if (main2 () != 12) + __builtin_abort (); + if (main3 () != 14) + __builtin_abort (); + if (main4() != 14) + __builtin_abort (); + return 0; +}
Hello Everyone, This patch will fix the bug reported in Bugzilla, PR 60586. The issue was that the spawned function's function arguments must not be pushed into the nested/lambda function. This patch should fix that issue. I have tested this on x86_64 (linux and Cygwin flavors). Is this OK for trunk? Here are the Changelog entries: gcc/c-family/ChangeLog 2015-08-31 Balaji V. Iyer <balaji.v.iyer@intel.com> PR middle-end/60586 * c-common.h: Added two more parameters to the gimplify_cilk_spawn function. * c-gimplify.c (c_gimplify_expr): Likewise. * cilk.c (gimplify_cilk_spawn): Likewise and called gimplify_call_params_in_spawned_fn. (gimplify_call_params_in_spawned_fn): New function. gcc/cp/ChangeLog 2015-08-31 Balaji V. Iyer <balaji.v.iyer@intel.com> PR middle-end/60586 * cp-gimplify.c (cp_gimplify_expr): Added two additional parameters to gimplify_cilk_spawn. gcc/testsuite/ChangeLog 2015-08-31 Balaji V. Iyer <balaji.v.iyer@intel.com> PR middle-end/60586 * c-c++-common/cilk-plus/CK/pr60586.c: New file. * g++.dg/cilk-plus/CK/pr60586.cc: Likewise. Thanks, Balaji V. Iyer.