[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307 --- Comment #10 from Jakub Jelinek --- Author: jakub Date: Wed Jan 21 21:23:04 2015 New Revision: 219969 URL: https://gcc.gnu.org/viewcvs?rev=219969&root=gcc&view=rev Log: 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 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. Added: trunk/gcc/testsuite/c-c++-common/cilk-plus/CK/pr63307.c Modified: trunk/gcc/c-family/ChangeLog trunk/gcc/c-family/cilk.c trunk/gcc/testsuite/ChangeLog
[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307 Richard Biener changed: What|Removed |Added Priority|P3 |P1
[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307 Jakub Jelinek changed: What|Removed |Added Target Milestone|4.9.2 |4.9.3 --- Comment #9 from Jakub Jelinek --- GCC 4.9.2 has been released.
[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307 Tobias Burnus changed: What|Removed |Added CC||burnus at gcc dot gnu.org --- Comment #8 from Tobias Burnus --- Note that the patch was reverted in r216502.
[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307 --- Comment #7 from Andrew Pinski --- (In reply to iverbin from comment #6) > Author: iverbin > Date: Mon Oct 20 15:22:09 2014 > New Revision: 216483 This breaks the build as wd->decl_map will always contain a BLOCK which does not have an UID. Please revert it as it is obvious you did not test it as a simple bootstrap (with checking enabled which is default on the trunk) would have found this issue.
[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307 --- Comment #6 from iverbin at gcc dot gnu.org --- Author: iverbin Date: Mon Oct 20 15:22:09 2014 New Revision: 216483 URL: https://gcc.gnu.org/viewcvs?rev=216483&root=gcc&view=rev Log: PR c/63307 gcc/c-family/ * 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. Modified: trunk/gcc/c-family/ChangeLog trunk/gcc/c-family/cilk.c
[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307 --- Comment #5 from Igor Zamyatin --- (In reply to Jakub Jelinek from comment #4) > I don't think so. They copy declarations, i.e. create new declarations, and > the different ordering of their DECL_UID values may result in code > generation differences (e.g. various other spots in the compiler sort based > on DECL_UIDs, > if you create them in pretty random order, you'll surely trigger some > -fcompare-debug (perhaps not with current limited testsuite coverage, but > with other tests). Right, thanks for the clarification. Will prepare the whole patch then
[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307 --- Comment #3 from Igor Zamyatin --- (In reply to Jakub Jelinek from comment #2) > > > + vec_arglist.release(); > > Formatting. You could use auto_vec, perhaps with some stack allocated > initial buffer if you think say 16 vector elements would be typically enough. Is it ok to have auto_vec declaration outside the routine? > > Also, what about all the remaining 3 callbacks that create or may create > decls and have the same problem? for_local_cb, wrapper_local_cb and > declare_one_free_variable. These are callbacks that seem to be safe in the sense of random ordering - perform some 1 to 1 mapping
[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307 --- Comment #4 from Jakub Jelinek --- (In reply to Igor Zamyatin from comment #3) > (In reply to Jakub Jelinek from comment #2) > > > > > > + vec_arglist.release(); > > > > Formatting. You could use auto_vec, perhaps with some stack allocated > > initial buffer if you think say 16 vector elements would be typically > > enough. > > Is it ok to have auto_vec declaration outside the routine? Why do you need to declare it outside of the routine? That seems undesirable to me. > > Also, what about all the remaining 3 callbacks that create or may create > > decls and have the same problem? for_local_cb, wrapper_local_cb and > > declare_one_free_variable. > > These are callbacks that seem to be safe in the sense of random ordering - > perform some 1 to 1 mapping I don't think so. They copy declarations, i.e. create new declarations, and the different ordering of their DECL_UID values may result in code generation differences (e.g. various other spots in the compiler sort based on DECL_UIDs, if you create them in pretty random order, you'll surely trigger some -fcompare-debug (perhaps not with current limited testsuite coverage, but with other tests).
[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307
--- Comment #2 from Jakub Jelinek ---
(In reply to Igor Zamyatin from comment #1)
> Would like to ask here first - will something like following be ok:
> +typedef struct
> +{
> + tree parm;
> + tree arg;
> +} decl_pair;
> +
> +static vec vec_arglist;
Just use struct cilk_decl_pair, no need for typedef in C++ here, and
try to avoid ODR issues. Also, why a static variable? Then you supposedly
would need to GTY handle it etc., which is IMHO undesirable.
> static bool
> wrapper_parm_cb (const void *key0, void **val0, void *data)
> {
> - struct wrapper_data *wd = (struct wrapper_data *) data;
>tree arg = * (tree *)&key0;
>tree val = (tree)*val0;
>tree parm;
> + decl_pair dp;
>
>if (val == error_mark_node || val == arg)
> return true;
> @@ -370,25 +379,48 @@ wrapper_parm_cb (const void *key0, void **val0, void
> *data)
> }
>else
> 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);
> +
> + dp.parm = parm;
> + dp.arg = arg;
> + vec_arglist.safe_push(dp);
Formatting, missing space before (. But more importantly, the diagnostics
still be in random order. So, I'd strongly suggest to move also the
diagnostics and ADDR_EXPR building etc. into a loop that walks the sorted
vector.
>return true;
> }
>
> /* This function is used to build a wrapper of a certain type. */
>
> +static int
> +compare_decls (const void *a, const void *b)
> +{
> +const decl_pair* t1 = (const decl_pair*) a;
> +const decl_pair* t2 = (const decl_pair*) b;
> +
> +return DECL_UID(t1->arg) > DECL_UID(t2->arg);
Formatting. Space before *, not after, space before (, indentation 2 columns
rather than 4.
> +}
> +
> static void
> build_wrapper_type (struct wrapper_data *wd)
> {
> + unsigned int j;
> + decl_pair * c;
>wd->arglist = NULL_TREE;
>wd->parms = NULL_TREE;
>wd->argtypes = void_list_node;
>
> - pointer_map_traverse (wd->decl_map, wrapper_parm_cb, wd);
> + vec_arglist.create (0);
> + pointer_map_traverse (wd->decl_map, wrapper_parm_cb, NULL);
>gcc_assert (wd->type != CILK_BLOCK_FOR);
>
> + vec_arglist.qsort(compare_decls);
Formatting.
> +
> + FOR_EACH_VEC_ELT (vec_arglist, j, c)
> +{
> + TREE_CHAIN (c->parm) = wd->parms;
> + wd->parms = c->parm;
> + wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (c->parm),
> wd->argtypes);
> + wd->arglist = tree_cons (NULL_TREE, c->arg, wd->arglist);
> +}
As I said earlier, I'd do diagnostics and ADDR_EXPR creation in this loop too.
> + vec_arglist.release();
Formatting. You could use auto_vec, perhaps with some stack allocated initial
buffer if you think say 16 vector elements would be typically enough.
Also, what about all the remaining 3 callbacks that create or may create decls
and have the same problem? for_local_cb, wrapper_local_cb and
declare_one_free_variable.
[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307
--- Comment #1 from Igor Zamyatin ---
Would like to ask here first - will something like following be ok:
diff --git a/gcc/c-family/cilk.c b/gcc/c-family/cilk.c
index bf549ad..f453bc5 100644
--- a/gcc/c-family/cilk.c
+++ b/gcc/c-family/cilk.c
@@ -36,6 +36,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,15 +333,23 @@ create_cilk_helper_decl (struct wrapper_data *wd)
return fndecl;
}
+typedef struct
+{
+ tree parm;
+ tree arg;
+} decl_pair;
+
+static vec vec_arglist;
+
/* A function used by walk tree to find wrapper parms. */
static bool
wrapper_parm_cb (const void *key0, void **val0, void *data)
{
- struct wrapper_data *wd = (struct wrapper_data *) data;
tree arg = * (tree *)&key0;
tree val = (tree)*val0;
tree parm;
+ decl_pair dp;
if (val == error_mark_node || val == arg)
return true;
@@ -370,25 +379,48 @@ wrapper_parm_cb (const void *key0, void **val0, void
*data)
}
else
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);
+
+ dp.parm = parm;
+ dp.arg = arg;
+ vec_arglist.safe_push(dp);
return true;
}
/* This function is used to build a wrapper of a certain type. */
+static int
+compare_decls (const void *a, const void *b)
+{
+const decl_pair* t1 = (const decl_pair*) a;
+const decl_pair* t2 = (const decl_pair*) b;
+
+return DECL_UID(t1->arg) > DECL_UID(t2->arg);
+}
+
static void
build_wrapper_type (struct wrapper_data *wd)
{
+ unsigned int j;
+ decl_pair * c;
wd->arglist = NULL_TREE;
wd->parms = NULL_TREE;
wd->argtypes = void_list_node;
- pointer_map_traverse (wd->decl_map, wrapper_parm_cb, wd);
+ vec_arglist.create (0);
+ pointer_map_traverse (wd->decl_map, wrapper_parm_cb, NULL);
gcc_assert (wd->type != CILK_BLOCK_FOR);
+ vec_arglist.qsort(compare_decls);
+
+ FOR_EACH_VEC_ELT (vec_arglist, j, c)
+{
+ TREE_CHAIN (c->parm) = wd->parms;
+ wd->parms = c->parm;
+ wd->argtypes = tree_cons (NULL_TREE, TREE_TYPE (c->parm), wd->argtypes);
+ wd->arglist = tree_cons (NULL_TREE, c->arg, wd->arglist);
+}
+ vec_arglist.release();
+
/* 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.
Bootstrapped successfully with GCC_COMPARE_DEBUG=1
[Bug c/63307] [4.9/5 Regression] Cilk+ breaks -fcompare-debug bootstrap
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63307 Richard Biener changed: What|Removed |Added Target Milestone|--- |4.9.2
