[PATCH, PR63307] Fix generation of new declarations in random order

[PATCH, PR63307] Fix generation of new declarations in random order

[Zamyatin, Igor]

Hi!

The following patch does fix random order for new decls generation during Cilk_spawn generation.
As Jakub suggested in the PR first we deal with vectors, then sort them and only then perform necessary generation of new decls.

Bootstrapped and regtested on trunk/4.9.
For trunk I couldn't check with COMPARE_DEBUG since building fails somewhere else. 
For 4.9 COMPARE_DEBUG building is ok.

Is it ok for trunk and backport into 4.9?


c-family/Changelog:

2014-10-03  Igor Zamyatin  <igor.zamyatin@intel.com>
 
	PR c/63307
	* cilk.c: Include vec.h.
	(struct cilk_decls): New structure.
	(wrapper_parm_cb): Split this function to...
	(fill_decls_vec): ...this...
	(create_parm_list): ...and this.
	(compare_decls): New function.
	(for_local_cb): Remove.
	(wrapper_local_cb): Ditto.
	(build_wrapper_type): For now first traverse and fill vector of
	declarations then sort it and then deal with sorted vector.
	(cilk_outline): Ditto.
	(declare_one_free_variable): Ditto.



diff --git a/gcc/c-family/cilk.c b/gcc/c-family/cilk.c
index 20b3432..e7a1c6a 100644
--- a/gcc/c-family/cilk.c
+++ b/gcc/c-family/cilk.c
@@ -35,6 +35,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "toplev.h" 
 #include "cgraph.h"
 #include "diagnostic.h"
+#include "vec.h"
 #include "cilk.h"
 
 enum add_variable_type {
@@ -332,17 +333,36 @@ create_cilk_helper_decl (struct wrapper_data *wd)
   return fndecl;
 }
 
-/* A function used by walk tree to find wrapper parms.  */
+struct cilk_decls
+{
+  tree key;
+  tree *val;
+};
+
+/* A function used by traversal to fill vector of decls for further work.  */
 
 bool
-wrapper_parm_cb (tree const &key0, tree *val0, wrapper_data *wd)
+fill_decls_vec (tree const &key0, tree *val0, auto_vec<struct cilk_decls> *v)
+{
+  tree t1 = key0;
+  struct cilk_decls dp;
+
+  dp.key = t1;
+  dp.val = val0;
+  v->safe_push (dp);
+  return true;
+}
+
+/* Function that actually creates necessary parm lists.  */
+
+static void
+create_parm_list (struct wrapper_data *wd, tree *val0, tree arg)
 {
-  tree arg = key0;
   tree val = *val0;
   tree parm;
 
   if (val == error_mark_node || val == arg)
-    return true;
+    return;
 
   if (TREE_CODE (val) == PAREN_EXPR)
     {
@@ -360,7 +380,7 @@ wrapper_parm_cb (tree const &key0, tree *val0, wrapper_data *wd)
        }
       else
        arg = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (arg)), arg);
-
+
       val = TREE_OPERAND (val, 0);
       *val0 = val;
       gcc_assert (TREE_CODE (val) == INDIRECT_REF);
@@ -371,9 +391,19 @@ wrapper_parm_cb (tree const &key0, tree *val0, wrapper_data *wd)
     parm = val;
   TREE_CHAIN (parm) = wd->parms;
   wd->parms = parm;
-  wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (parm), wd->argtypes); 
-  wd->arglist = tree_cons (NULL_TREE, arg, wd->arglist); 
-  return true;
+  wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (parm), wd->argtypes);
+  wd->arglist = tree_cons (NULL_TREE, arg, wd->arglist);
+}
+
+/* Sorting decls in a vector.  */
+
+static int
+compare_decls (const void *a, const void *b)
+{
+  const struct cilk_decls* t1 = (const struct cilk_decls*) a;
+  const struct cilk_decls* t2 = (const struct cilk_decls*) b;
+
+  return DECL_UID (t1->key) > DECL_UID (t2->key);
 }
 
 /* This function is used to build a wrapper of a certain type.  */
@@ -381,13 +411,21 @@ wrapper_parm_cb (tree const &key0, tree *val0, wrapper_data *wd)
 static void
 build_wrapper_type (struct wrapper_data *wd)
 {
+  unsigned int j;
+  struct cilk_decls * c;
+  auto_vec<struct cilk_decls> vd;
   wd->arglist = NULL_TREE;
   wd->parms = NULL_TREE;
   wd->argtypes = void_list_node;
 
-  wd->decl_map->traverse<wrapper_data *, wrapper_parm_cb> (wd);
+  wd->decl_map->traverse<auto_vec<struct cilk_decls> *, fill_decls_vec> (&vd);
   gcc_assert (wd->type != CILK_BLOCK_FOR);
 
+  vd.qsort (compare_decls);
+
+  FOR_EACH_VEC_ELT (vd, j, c)
+   create_parm_list (wd, c->val, c->key);
+
   /* Now build a function.
      Its return type is void (all side effects are via explicit parameters).
      Its parameters are WRAPPER_PARMS with type WRAPPER_TYPES.
@@ -448,41 +486,19 @@ copy_decl_for_cilk (tree decl, copy_body_data *id)
     }
 }
 
-/* Copy all local variables.  */
-
-bool
-for_local_cb (tree const &k, tree *vp, copy_body_data *id)
-{
-  tree v = *vp;
-
-  if (v == error_mark_node)
-    *vp = copy_decl_no_change (k, id);
-  return true;
-}
-
-/* Copy all local declarations from a _Cilk_spawned function's body.  */
-
-bool
-wrapper_local_cb (tree const &key, tree *vp, copy_body_data *id)
-{
-  tree val = *vp;
-
-  if (val == error_mark_node)
-    *vp = copy_decl_for_cilk (key, id);
-
-  return true;
-}
-
 /* Alter a tree STMT from OUTER_FN to form the body of INNER_FN.  */
 
 void
 cilk_outline (tree inner_fn, tree *stmt_p, void *w)
 {
   struct wrapper_data *wd = (struct wrapper_data *) w;
-  const tree outer_fn = wd->context;         
+  const tree outer_fn = wd->context;
   const bool nested = (wd->type == CILK_BLOCK_FOR);
   copy_body_data id;
   bool throws;
+  auto_vec<struct cilk_decls> vd;
+  unsigned int j;
+  struct cilk_decls * c;
 
   DECL_STATIC_CHAIN (outer_fn) = 1;
 
@@ -508,11 +524,13 @@ cilk_outline (tree inner_fn, tree *stmt_p, void *w)
 
   insert_decl_map (&id, wd->block, DECL_INITIAL (inner_fn));
 
+  wd->decl_map->traverse<auto_vec<struct cilk_decls> *, fill_decls_vec> (&vd);
+  vd.qsort (compare_decls);
   /* We don't want the private variables any more.  */
-  if (nested)
-    wd->decl_map->traverse<copy_body_data *, for_local_cb> (&id);
-  else
-    wd->decl_map->traverse<copy_body_data *, wrapper_local_cb> (&id);
+  FOR_EACH_VEC_ELT (vd, j, c)
+   if (*(c->val) == error_mark_node)
+     *(c->val) = nested ? copy_decl_no_change (c->key, &id)
+                       : copy_decl_for_cilk (c->key, &id);
 
   walk_tree (stmt_p, copy_tree_body_r, (void *) &id, NULL);
 
@@ -617,7 +635,7 @@ free_wd (struct wrapper_data *wd)
 */
 
 bool
-declare_one_free_variable (tree const &var0, tree *map0, wrapper_data &)
+declare_one_free_variable (tree var0, tree *map0)
 {
   const_tree var = var0;
   tree map = *map0;
@@ -690,6 +708,9 @@ create_cilk_wrapper (tree exp, tree *args_out)
 {
   struct wrapper_data wd;
   tree fndecl;
+  unsigned int j;
+  struct cilk_decls * c;
+  auto_vec<struct cilk_decls> vd;
 
   init_wd (&wd, CILK_BLOCK_SPAWN);
 
@@ -710,7 +731,11 @@ create_cilk_wrapper (tree exp, tree *args_out)
     }
   else
     extract_free_variables (exp, &wd, ADD_READ);
-  wd.decl_map->traverse<wrapper_data &, declare_one_free_variable> (wd);
+  wd.decl_map->traverse<auto_vec<struct cilk_decls> *, fill_decls_vec> (&vd);
+  vd.qsort (compare_decls);
+  FOR_EACH_VEC_ELT (vd, j, c)
+   declare_one_free_variable (c->key, c->val);
+
   wd.block = TREE_BLOCK (exp);
   if (!wd.block)
     wd.block = DECL_INITIAL (current_function_decl);

Re: [PATCH, PR63307] Fix generation of new declarations in random order

[Jeff Law]

On 10/04/14 05:54, Zamyatin, Igor wrote:
> Hi!
>
> The following patch does fix random order for new decls generation during Cilk_spawn generation.
> As Jakub suggested in the PR first we deal with vectors, then sort them and only then perform necessary generation of new decls.
>
> Bootstrapped and regtested on trunk/4.9.
> For trunk I couldn't check with COMPARE_DEBUG since building fails somewhere else.
> For 4.9 COMPARE_DEBUG building is ok.
>
> Is it ok for trunk and backport into 4.9?
>
>
> c-family/Changelog:
>
> 2014-10-03  Igor Zamyatin  <igor.zamyatin@intel.com>
>
> 	PR c/63307
> 	* cilk.c: Include vec.h.
> 	(struct cilk_decls): New structure.
> 	(wrapper_parm_cb): Split this function to...
> 	(fill_decls_vec): ...this...
> 	(create_parm_list): ...and this.
> 	(compare_decls): New function.
> 	(for_local_cb): Remove.
> 	(wrapper_local_cb): Ditto.
> 	(build_wrapper_type): For now first traverse and fill vector of
> 	declarations then sort it and then deal with sorted vector.
> 	(cilk_outline): Ditto.
> 	(declare_one_free_variable): Ditto.
OK for the trunk.  No sure if Jakub wants to backport to 4.9 or not. 
That'd be his call.

> +
> +static int
> +compare_decls (const void *a, const void *b)
> +{
> +  const struct cilk_decls* t1 = (const struct cilk_decls*) a;
> +  const struct cilk_decls* t2 = (const struct cilk_decls*) b;
> +
> +  return DECL_UID (t1->key) > DECL_UID (t2->key);
We really prefer fully specified sorts.   For a qsort callback, this 
doesn't look fully specified.


With that fixed, this should be OK.

jeff

RE: [PATCH, PR63307] Fix generation of new declarations in random order

[Zamyatin, Igor]

> >
> >
> > c-family/Changelog:
> >
> > 2014-10-03  Igor Zamyatin  <igor.zamyatin@intel.com>
> >
> > 	PR c/63307
> > 	* cilk.c: Include vec.h.
> > 	(struct cilk_decls): New structure.
> > 	(wrapper_parm_cb): Split this function to...
> > 	(fill_decls_vec): ...this...
> > 	(create_parm_list): ...and this.
> > 	(compare_decls): New function.
> > 	(for_local_cb): Remove.
> > 	(wrapper_local_cb): Ditto.
> > 	(build_wrapper_type): For now first traverse and fill vector of
> > 	declarations then sort it and then deal with sorted vector.
> > 	(cilk_outline): Ditto.
> > 	(declare_one_free_variable): Ditto.
> OK for the trunk.  No sure if Jakub wants to backport to 4.9 or not.
> That'd be his call.

Sure, I have also a patch for 4.9. 

> 
> > +
> > +static int
> > +compare_decls (const void *a, const void *b) {
> > +  const struct cilk_decls* t1 = (const struct cilk_decls*) a;
> > +  const struct cilk_decls* t2 = (const struct cilk_decls*) b;
> > +
> > +  return DECL_UID (t1->key) > DECL_UID (t2->key);
> We really prefer fully specified sorts.   For a qsort callback, this
> doesn't look fully specified.
> 
> 
> With that fixed, this should be OK.
> 
> jeff

Thanks for the review. Here is the updated version. 
Is it ok?

diff --git a/gcc/c-family/cilk.c b/gcc/c-family/cilk.c
index 20b3432..ffcf3b5 100644
--- a/gcc/c-family/cilk.c
+++ b/gcc/c-family/cilk.c
@@ -35,6 +35,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "toplev.h" 
 #include "cgraph.h"
 #include "diagnostic.h"
+#include "vec.h"
 #include "cilk.h"
 
 enum add_variable_type {
@@ -332,17 +333,36 @@ create_cilk_helper_decl (struct wrapper_data *wd)
   return fndecl;
 }
 
-/* A function used by walk tree to find wrapper parms.  */
+struct cilk_decls
+{
+  tree key;
+  tree *val;
+};
+
+/* A function used by traversal to fill vector of decls for further work.  */
 
 bool
-wrapper_parm_cb (tree const &key0, tree *val0, wrapper_data *wd)
+fill_decls_vec (tree const &key0, tree *val0, auto_vec<struct cilk_decls> *v)
+{
+  tree t1 = key0;
+  struct cilk_decls dp;
+
+  dp.key = t1;
+  dp.val = val0;
+  v->safe_push (dp);
+  return true;
+}
+
+/* Function that actually creates necessary parm lists.  */
+
+static void
+create_parm_list (struct wrapper_data *wd, tree *val0, tree arg)
 {
-  tree arg = key0;
   tree val = *val0;
   tree parm;
 
   if (val == error_mark_node || val == arg)
-    return true;
+    return;
 
   if (TREE_CODE (val) == PAREN_EXPR)
     {
@@ -360,7 +380,7 @@ wrapper_parm_cb (tree const &key0, tree *val0, wrapper_data *wd)
        }
       else
        arg = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (arg)), arg);
-
+
       val = TREE_OPERAND (val, 0);
       *val0 = val;
       gcc_assert (TREE_CODE (val) == INDIRECT_REF);
@@ -371,9 +391,24 @@ wrapper_parm_cb (tree const &key0, tree *val0, wrapper_data *wd)
     parm = val;
   TREE_CHAIN (parm) = wd->parms;
   wd->parms = parm;
-  wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (parm), wd->argtypes); 
-  wd->arglist = tree_cons (NULL_TREE, arg, wd->arglist); 
-  return true;
+  wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (parm), wd->argtypes);
+  wd->arglist = tree_cons (NULL_TREE, arg, wd->arglist);
+}
+
+/* Sorting decls in a vector.  */
+
+static int
+compare_decls (const void *a, const void *b)
+{
+  const struct cilk_decls* t1 = (const struct cilk_decls*) a;
+  const struct cilk_decls* t2 = (const struct cilk_decls*) b;
+
+  if (DECL_UID (t1->key) > DECL_UID (t2->key))
+    return 1;
+  else if (DECL_UID (t1->key) < DECL_UID (t2->key))
+    return -1;
+  else
+    return 0;
 }
 
 /* This function is used to build a wrapper of a certain type.  */
@@ -381,13 +416,21 @@ wrapper_parm_cb (tree const &key0, tree *val0, wrapper_data *wd)
 static void
 build_wrapper_type (struct wrapper_data *wd)
 {
+  unsigned int j;
+  struct cilk_decls * c;
+  auto_vec<struct cilk_decls> vd;
   wd->arglist = NULL_TREE;
   wd->parms = NULL_TREE;
   wd->argtypes = void_list_node;
 
-  wd->decl_map->traverse<wrapper_data *, wrapper_parm_cb> (wd);
+  wd->decl_map->traverse<auto_vec<struct cilk_decls> *, fill_decls_vec> (&vd);
   gcc_assert (wd->type != CILK_BLOCK_FOR);
 
+  vd.qsort (compare_decls);
+
+  FOR_EACH_VEC_ELT (vd, j, c)
+   create_parm_list (wd, c->val, c->key);
+
   /* Now build a function.
      Its return type is void (all side effects are via explicit parameters).
      Its parameters are WRAPPER_PARMS with type WRAPPER_TYPES.
@@ -448,41 +491,19 @@ copy_decl_for_cilk (tree decl, copy_body_data *id)
     }
 }
 
-/* Copy all local variables.  */
-
-bool
-for_local_cb (tree const &k, tree *vp, copy_body_data *id)
-{
-  tree v = *vp;
-
-  if (v == error_mark_node)
-    *vp = copy_decl_no_change (k, id);
-  return true;
-}
-
-/* Copy all local declarations from a _Cilk_spawned function's body.  */
-
-bool
-wrapper_local_cb (tree const &key, tree *vp, copy_body_data *id)
-{
-  tree val = *vp;
-
-  if (val == error_mark_node)
-    *vp = copy_decl_for_cilk (key, id);
-
-  return true;
-}
-
 /* Alter a tree STMT from OUTER_FN to form the body of INNER_FN.  */
 
 void
 cilk_outline (tree inner_fn, tree *stmt_p, void *w)
 {
   struct wrapper_data *wd = (struct wrapper_data *) w;
-  const tree outer_fn = wd->context;         
+  const tree outer_fn = wd->context;
   const bool nested = (wd->type == CILK_BLOCK_FOR);
   copy_body_data id;
   bool throws;
+  auto_vec<struct cilk_decls> vd;
+  unsigned int j;
+  struct cilk_decls * c;
 
   DECL_STATIC_CHAIN (outer_fn) = 1;
 
@@ -508,11 +529,13 @@ cilk_outline (tree inner_fn, tree *stmt_p, void *w)
 
   insert_decl_map (&id, wd->block, DECL_INITIAL (inner_fn));
 
+  wd->decl_map->traverse<auto_vec<struct cilk_decls> *, fill_decls_vec> (&vd);
+  vd.qsort (compare_decls);
   /* We don't want the private variables any more.  */
-  if (nested)
-    wd->decl_map->traverse<copy_body_data *, for_local_cb> (&id);
-  else
-    wd->decl_map->traverse<copy_body_data *, wrapper_local_cb> (&id);
+  FOR_EACH_VEC_ELT (vd, j, c)
+   if (*(c->val) == error_mark_node)
+     *(c->val) = nested ? copy_decl_no_change (c->key, &id)
+                       : copy_decl_for_cilk (c->key, &id);
 
   walk_tree (stmt_p, copy_tree_body_r, (void *) &id, NULL);
 
@@ -617,7 +640,7 @@ free_wd (struct wrapper_data *wd)
 */
 
 bool
-declare_one_free_variable (tree const &var0, tree *map0, wrapper_data &)
+declare_one_free_variable (tree var0, tree *map0)
 {
   const_tree var = var0;
   tree map = *map0;
@@ -690,6 +713,9 @@ create_cilk_wrapper (tree exp, tree *args_out)
 {
   struct wrapper_data wd;
   tree fndecl;
+  unsigned int j;
+  struct cilk_decls * c;
+  auto_vec<struct cilk_decls> vd;
 
   init_wd (&wd, CILK_BLOCK_SPAWN);
 
@@ -710,7 +736,11 @@ create_cilk_wrapper (tree exp, tree *args_out)
     }
   else
     extract_free_variables (exp, &wd, ADD_READ);
-  wd.decl_map->traverse<wrapper_data &, declare_one_free_variable> (wd);
+  wd.decl_map->traverse<auto_vec<struct cilk_decls> *, fill_decls_vec> (&vd);
+  vd.qsort (compare_decls);
+  FOR_EACH_VEC_ELT (vd, j, c)
+   declare_one_free_variable (c->key, c->val);
+
   wd.block = TREE_BLOCK (exp);
   if (!wd.block)
     wd.block = DECL_INITIAL (current_function_decl);

Thanks,
Igor

Re: [PATCH, PR63307] Fix generation of new declarations in random order

[Jeff Law]

On 10/16/14 09:16, Zamyatin, Igor wrote:
>>>
>>>
>>> c-family/Changelog:
>>>
>>> 2014-10-03  Igor Zamyatin  <igor.zamyatin@intel.com>
>>>
>>> 	PR c/63307
>>> 	* cilk.c: Include vec.h.
>>> 	(struct cilk_decls): New structure.
>>> 	(wrapper_parm_cb): Split this function to...
>>> 	(fill_decls_vec): ...this...
>>> 	(create_parm_list): ...and this.
>>> 	(compare_decls): New function.
>>> 	(for_local_cb): Remove.
>>> 	(wrapper_local_cb): Ditto.
>>> 	(build_wrapper_type): For now first traverse and fill vector of
>>> 	declarations then sort it and then deal with sorted vector.
>>> 	(cilk_outline): Ditto.
>>> 	(declare_one_free_variable): Ditto.
>> OK for the trunk.  No sure if Jakub wants to backport to 4.9 or not.
>> That'd be his call.
>
> Sure, I have also a patch for 4.9.
>
>>
>>> +
>>> +static int
>>> +compare_decls (const void *a, const void *b) {
>>> +  const struct cilk_decls* t1 = (const struct cilk_decls*) a;
>>> +  const struct cilk_decls* t2 = (const struct cilk_decls*) b;
>>> +
>>> +  return DECL_UID (t1->key) > DECL_UID (t2->key);
>> We really prefer fully specified sorts.   For a qsort callback, this
>> doesn't look fully specified.
>>
>>
>> With that fixed, this should be OK.
>>
>> jeff
>
> Thanks for the review. Here is the updated version.
> Is it ok?
Yes, this is good for the trunk.

Thanks,
Jeff

Re: [PATCH, PR63307] Fix generation of new declarations in random order

[Jakub Jelinek]

On Thu, Oct 16, 2014 at 11:06:34AM -0600, Jeff Law wrote:
> >>We really prefer fully specified sorts.   For a qsort callback, this
> >>doesn't look fully specified.
> >>
> >>
> >>With that fixed, this should be OK.
> >>
> >>jeff
> >
> >Thanks for the review. Here is the updated version.
> >Is it ok?
> Yes, this is good for the trunk.

This broke bootstrap everywhere unfortunately, has it been tested at all?

I already wrote during the initial comment that BLOCKs aren't decls and
you can't push them into the vectors, they can't be sorted easily
(BLOCK_NUMBER isn't assigned at that point e.g. and the comparison function
looks at DECL_UID unconditionally anyway).

I've bootstrapped/regtested on i686-linux the following quick fix,
bootstrapped on x86_64-linux too, in the middle of regtesting there.
If it succeeds, I'll commit as obvious, so that people can continue working
on the trunk.

2014-10-21  Jakub Jelinek  <jakub@redhat.com>

	* cilk.c (fill_decls_vec): Only put decls into vector v.
	(compare_decls): Fix up formatting.

--- gcc/c-family/cilk.c.jj	2014-10-20 19:24:54.000000000 +0200
+++ gcc/c-family/cilk.c	2014-10-21 08:46:24.727790990 +0200
@@ -347,9 +347,12 @@ fill_decls_vec (tree const &key0, tree *
   tree t1 = key0;
   struct cilk_decls dp;
 
-  dp.key = t1;
-  dp.val = val0;
-  v->safe_push (dp);
+  if (DECL_P (t1))
+    {
+      dp.key = t1;
+      dp.val = val0;
+      v->safe_push (dp);
+    }
   return true;
 }
 
@@ -400,8 +403,8 @@ create_parm_list (struct wrapper_data *w
 static int
 compare_decls (const void *a, const void *b)
 {
-  const struct cilk_decls* t1 = (const struct cilk_decls*) a;
-  const struct cilk_decls* t2 = (const struct cilk_decls*) b;
+  const struct cilk_decls *t1 = (const struct cilk_decls *) a;
+  const struct cilk_decls *t2 = (const struct cilk_decls *) b;
 
   if (DECL_UID (t1->key) > DECL_UID (t2->key))
     return 1;


	Jakub

RE: [PATCH, PR63307] Fix generation of new declarations in random order

[Zamyatin, Igor]

For some reasons it passed bootstrap locally...

> -----Original Message-----
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Tuesday, October 21, 2014 12:15 PM
> To: Zamyatin, Igor; Jeff Law
> Cc: GCC Patches (gcc-patches@gcc.gnu.org)
> Subject: Re: [PATCH, PR63307] Fix generation of new declarations in random
> order
> 
> On Thu, Oct 16, 2014 at 11:06:34AM -0600, Jeff Law wrote:
> > >>We really prefer fully specified sorts.   For a qsort callback, this
> > >>doesn't look fully specified.
> > >>
> > >>
> > >>With that fixed, this should be OK.
> > >>
> > >>jeff
> > >
> > >Thanks for the review. Here is the updated version.
> > >Is it ok?
> > Yes, this is good for the trunk.
> 
> This broke bootstrap everywhere unfortunately, has it been tested at all?
> 
> I already wrote during the initial comment that BLOCKs aren't decls and you
> can't push them into the vectors, they can't be sorted easily
> (BLOCK_NUMBER isn't assigned at that point e.g. and the comparison
> function looks at DECL_UID unconditionally anyway).
> 
> I've bootstrapped/regtested on i686-linux the following quick fix,
> bootstrapped on x86_64-linux too, in the middle of regtesting there.
> If it succeeds, I'll commit as obvious, so that people can continue working on
> the trunk.
> 
> 2014-10-21  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* cilk.c (fill_decls_vec): Only put decls into vector v.
> 	(compare_decls): Fix up formatting.
> 
> --- gcc/c-family/cilk.c.jj	2014-10-20 19:24:54.000000000 +0200
> +++ gcc/c-family/cilk.c	2014-10-21 08:46:24.727790990 +0200
> @@ -347,9 +347,12 @@ fill_decls_vec (tree const &key0, tree *
>    tree t1 = key0;
>    struct cilk_decls dp;
> 
> -  dp.key = t1;
> -  dp.val = val0;
> -  v->safe_push (dp);
> +  if (DECL_P (t1))
> +    {
> +      dp.key = t1;
> +      dp.val = val0;
> +      v->safe_push (dp);
> +    }
>    return true;
>  }
> 
> @@ -400,8 +403,8 @@ create_parm_list (struct wrapper_data *w  static int
> compare_decls (const void *a, const void *b)  {
> -  const struct cilk_decls* t1 = (const struct cilk_decls*) a;
> -  const struct cilk_decls* t2 = (const struct cilk_decls*) b;
> +  const struct cilk_decls *t1 = (const struct cilk_decls *) a;  const
> + struct cilk_decls *t2 = (const struct cilk_decls *) b;
> 
>    if (DECL_UID (t1->key) > DECL_UID (t2->key))
>      return 1;
> 
> 
> 	Jakub

Re: [PATCH, PR63307] Fix generation of new declarations in random order

[Jakub Jelinek]

On Tue, Oct 21, 2014 at 10:14:56AM +0200, Jakub Jelinek wrote:
> I've bootstrapped/regtested on i686-linux the following quick fix,
> bootstrapped on x86_64-linux too, in the middle of regtesting there.
> If it succeeds, I'll commit as obvious, so that people can continue working
> on the trunk.

Ah, Kyrill has reverted the commit in the mean time, so there is no rush for
this, so I'm not going to commit it now.

The question remains, are the decls all you need from the traversal (i.e.
what you need to act upon)?  From my earlier skim of the original code that
wasn't that obvious.
You can have in decl_map at least also BLOCKs, perhaps types too, what else?

> 2014-10-21  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* cilk.c (fill_decls_vec): Only put decls into vector v.
> 	(compare_decls): Fix up formatting.

	Jakub

RE: [PATCH, PR63307] Fix generation of new declarations in random order

[Zamyatin, Igor]

> 
> 
> The question remains, are the decls all you need from the traversal (i.e.
> what you need to act upon)?  From my earlier skim of the original code that
> wasn't that obvious.
> You can have in decl_map at least also BLOCKs, perhaps types too, what
> else?

Jakub,

Seems the BLOCKs are the only exception, they can be added in map by
insert_decl_map (&id, wd->block, DECL_INITIAL (inner_fn)); in cilk_outline

In other cases adding to decl_map is being done through add_variable routine which is called only for DECLs (in extract_free_variables)

Your fix for bootstrap looks correct since in cilk_outline we deal with error_mark_node values which are set only for DECLs.


Thanks,
Igor

Re: [PATCH, PR63307] Fix generation of new declarations in random order

[Jakub Jelinek]

On Wed, Oct 29, 2014 at 04:05:27PM +0000, Zamyatin, Igor wrote:
> > 
> > 
> > The question remains, are the decls all you need from the traversal (i.e.
> > what you need to act upon)?  From my earlier skim of the original code that
> > wasn't that obvious.
> > You can have in decl_map at least also BLOCKs, perhaps types too, what
> > else?
> 
> Jakub,
> 
> Seems the BLOCKs are the only exception, they can be added in map by
> insert_decl_map (&id, wd->block, DECL_INITIAL (inner_fn)); in cilk_outline
> 
> In other cases adding to decl_map is being done through add_variable routine which is called only for DECLs (in extract_free_variables)
> 
> Your fix for bootstrap looks correct since in cilk_outline we deal with error_mark_node values which are set only for DECLs.

I've reapplied your patch together with my fix and a new testcase for it
after bootstrapping/regtesting it on x86_64-linux and i686-linux.

2015-01-21  Jakub Jelinek  <jakub@redhat.com>

	PR c/63307
	* cilk.c (fill_decls_vec): Only put decls into vector v.                                                                                   
	(compare_decls): Fix up formatting.

	* c-c++-common/cilk-plus/CK/pr63307.c: New test.

2015-01-21  Igor Zamyatin  <igor.zamyatin@intel.com>

	PR c/63307
	* cilk.c: Include vec.h.
	(struct cilk_decls): New structure.
	(wrapper_parm_cb): Split this function to...
	(fill_decls_vec): ...this...
	(create_parm_list): ...and this.
	(compare_decls): New function.
	(for_local_cb): Remove.
	(wrapper_local_cb): Ditto.
	(build_wrapper_type): For now first traverse and fill vector of
	declarations then sort it and then deal with sorted vector.
	(cilk_outline): Ditto.
	(declare_one_free_variable): Ditto.

--- gcc/c-family/cilk.c.jj	2015-01-09 21:59:29.417205823 +0100
+++ gcc/c-family/cilk.c	2015-01-21 15:21:43.815378942 +0100
@@ -58,6 +58,7 @@ along with GCC; see the file COPYING3.
 #include "ipa-ref.h"
 #include "cgraph.h"
 #include "diagnostic.h"
+#include "vec.h"
 #include "cilk.h"
 
 enum add_variable_type {
@@ -355,17 +356,39 @@ create_cilk_helper_decl (struct wrapper_
   return fndecl;
 }
 
-/* A function used by walk tree to find wrapper parms.  */
+struct cilk_decls
+{
+  tree key;
+  tree *val;
+};
+
+/* A function used by traversal to fill vector of decls for further work.  */
 
 bool
-wrapper_parm_cb (tree const &key0, tree *val0, wrapper_data *wd)
+fill_decls_vec (tree const &key0, tree *val0, auto_vec<struct cilk_decls> *v)
+{
+  tree t1 = key0;
+  struct cilk_decls dp;
+
+  if (DECL_P (t1))
+    {
+      dp.key = t1;
+      dp.val = val0;
+      v->safe_push (dp);
+    }
+  return true;
+}
+
+/* Function that actually creates necessary parm lists.  */
+
+static void
+create_parm_list (struct wrapper_data *wd, tree *val0, tree arg)
 {
-  tree arg = key0;
   tree val = *val0;
   tree parm;
 
   if (val == error_mark_node || val == arg)
-    return true;
+    return;
 
   if (TREE_CODE (val) == PAREN_EXPR)
     {
@@ -383,7 +406,7 @@ wrapper_parm_cb (tree const &key0, tree
 	}
       else
 	arg = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (arg)), arg);
-	
+
       val = TREE_OPERAND (val, 0);
       *val0 = val;
       gcc_assert (TREE_CODE (val) == INDIRECT_REF);
@@ -394,9 +417,24 @@ wrapper_parm_cb (tree const &key0, tree
     parm = val;
   TREE_CHAIN (parm) = wd->parms;
   wd->parms = parm;
-  wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (parm), wd->argtypes); 
-  wd->arglist = tree_cons (NULL_TREE, arg, wd->arglist); 
-  return true;
+  wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (parm), wd->argtypes);
+  wd->arglist = tree_cons (NULL_TREE, arg, wd->arglist);
+}
+
+/* Sorting decls in a vector.  */
+
+static int
+compare_decls (const void *a, const void *b)
+{
+  const struct cilk_decls *t1 = (const struct cilk_decls *) a;
+  const struct cilk_decls *t2 = (const struct cilk_decls *) b;
+
+  if (DECL_UID (t1->key) > DECL_UID (t2->key))
+    return 1;
+  else if (DECL_UID (t1->key) < DECL_UID (t2->key))
+    return -1;
+  else
+    return 0;
 }
 
 /* This function is used to build a wrapper of a certain type.  */
@@ -404,12 +442,19 @@ wrapper_parm_cb (tree const &key0, tree
 static void
 build_wrapper_type (struct wrapper_data *wd)
 {
+  unsigned int j;
+  struct cilk_decls * c;
+  auto_vec<struct cilk_decls> vd;
   wd->arglist = NULL_TREE;
   wd->parms = NULL_TREE;
   wd->argtypes = void_list_node;
 
-  wd->decl_map->traverse<wrapper_data *, wrapper_parm_cb> (wd);
   gcc_assert (wd->type != CILK_BLOCK_FOR);
+  wd->decl_map->traverse<auto_vec<struct cilk_decls> *, fill_decls_vec> (&vd);
+  vd.qsort (compare_decls);
+
+  FOR_EACH_VEC_ELT (vd, j, c)
+   create_parm_list (wd, c->val, c->key);
 
   /* Now build a function.
      Its return type is void (all side effects are via explicit parameters).
@@ -471,41 +516,19 @@ copy_decl_for_cilk (tree decl, copy_body
     }
 }
 
-/* Copy all local variables.  */
-
-bool
-for_local_cb (tree const &k, tree *vp, copy_body_data *id)
-{
-  tree v = *vp;
-
-  if (v == error_mark_node)
-    *vp = copy_decl_no_change (k, id);
-  return true;
-}
-
-/* Copy all local declarations from a _Cilk_spawned function's body.  */
-
-bool
-wrapper_local_cb (tree const &key, tree *vp, copy_body_data *id)
-{
-  tree val = *vp;
-
-  if (val == error_mark_node)
-    *vp = copy_decl_for_cilk (key, id);
-
-  return true;
-}
-
 /* Alter a tree STMT from OUTER_FN to form the body of INNER_FN.  */
 
 void
 cilk_outline (tree inner_fn, tree *stmt_p, void *w)
 {
   struct wrapper_data *wd = (struct wrapper_data *) w;
-  const tree outer_fn = wd->context;	      
+  const tree outer_fn = wd->context;
   const bool nested = (wd->type == CILK_BLOCK_FOR);
   copy_body_data id;
   bool throws;
+  auto_vec<struct cilk_decls> vd;
+  unsigned int j;
+  struct cilk_decls * c;
 
   DECL_STATIC_CHAIN (outer_fn) = 1;
 
@@ -531,11 +554,13 @@ cilk_outline (tree inner_fn, tree *stmt_
 
   insert_decl_map (&id, wd->block, DECL_INITIAL (inner_fn));
 
+  wd->decl_map->traverse<auto_vec<struct cilk_decls> *, fill_decls_vec> (&vd);
+  vd.qsort (compare_decls);
   /* We don't want the private variables any more.  */
-  if (nested)
-    wd->decl_map->traverse<copy_body_data *, for_local_cb> (&id);
-  else
-    wd->decl_map->traverse<copy_body_data *, wrapper_local_cb> (&id);
+  FOR_EACH_VEC_ELT (vd, j, c)
+   if (*(c->val) == error_mark_node)
+     *(c->val) = nested ? copy_decl_no_change (c->key, &id)
+			: copy_decl_for_cilk (c->key, &id);
 
   walk_tree (stmt_p, copy_tree_body_r, (void *) &id, NULL);
 
@@ -640,7 +665,7 @@ free_wd (struct wrapper_data *wd)
 */
 
 bool
-declare_one_free_variable (tree const &var0, tree *map0, wrapper_data &)
+declare_one_free_variable (tree var0, tree *map0)
 {
   const_tree var = var0;
   tree map = *map0;
@@ -713,6 +738,9 @@ create_cilk_wrapper (tree exp, tree *arg
 {
   struct wrapper_data wd;
   tree fndecl;
+  unsigned int j;
+  struct cilk_decls * c;
+  auto_vec<struct cilk_decls> vd;
 
   init_wd (&wd, CILK_BLOCK_SPAWN);
 
@@ -733,7 +761,11 @@ create_cilk_wrapper (tree exp, tree *arg
     }
   else
     extract_free_variables (exp, &wd, ADD_READ);
-  wd.decl_map->traverse<wrapper_data &, declare_one_free_variable> (wd);
+  wd.decl_map->traverse<auto_vec<struct cilk_decls> *, fill_decls_vec> (&vd);
+  vd.qsort (compare_decls);
+  FOR_EACH_VEC_ELT (vd, j, c)
+   declare_one_free_variable (c->key, c->val);
+
   wd.block = TREE_BLOCK (exp);
   if (!wd.block)
     wd.block = DECL_INITIAL (current_function_decl);
--- gcc/testsuite/c-c++-common/cilk-plus/CK/pr63307.c.jj	2015-01-21 14:57:39.676438860 +0100
+++ gcc/testsuite/c-c++-common/cilk-plus/CK/pr63307.c	2015-01-21 14:57:34.527528152 +0100
@@ -0,0 +1,4 @@
+/* { dg-options "-fcilkplus -fcompare-debug" } */
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+
+#include "fib_no_return.c"


	Jakub