I've just tried a -fcompare-debug bootstrap, and one of the two reasons (other was Ada) that it failed is Cilk+: ../configure --enable-languages=all,ada,obj-c++,lto,go --enable-checking=release; GCC_COMPARE_DEBUG=1 make -j48 bootstrap > LOG 2>&1 && GCC_COMPARE_DEBUG=1 make -j48 -k check > LOGC 2>&1; ../contrib/test_summary > LOGT 2>&1 ... xg++: error: ../../../libcilkrts/runtime/cilk-abi-cilk-for.cpp: -fcompare-debug failure (length) From what I can see, the bug is (at least) in c-family/cilk.c, where it seems in all 4 traverse handlers on decl_map it creates decls, adds fields etc. That is a big NO NO in GCC, hash table traversal order should never affect code generation. So, seeing changes like: - _cilk_spn_1 (low, grain, mid, body, loop_root_pedigree, data, w); + _cilk_spn_1 (data, body, loop_root_pedigree, w, mid, low, grain); or: -<built-in> (unsigned int D.3190, int D.3189, unsigned int D.3188, void (*<T39b>) (void *, unsigned int, unsigned int) D.3187, struct __cilkrts_pedigree * D.3186, void * D.3185, struct __cilkrts_worker * D.3184) +<built-in> (void * D.3190, void (*<T39b>) (void *, unsigned int, unsigned int) D.3189, struct __cilkrts_pedigree * D.3188, struct __cilkrts_worker * D.3187, unsigned int D.3186, unsigned int D.3185, int D.3184) in *.gimple dump between -g and -g0 is just wrong (and in this case it isn't really debug related, just different order in the hash map). If you don't have the decls (but, apparently you put also BLOCKs in there, what else?) you want to traverse in some other data structure, you can e.g. traverse the hash table but only collect the decls (do you need anything else, e.g. the BLOCKs?, types, etc.) you saw into some vector, then qsort the vector (for decls e.g. by DECL_UID, if you have also other trees, it might be harder) and then traverse the sorted vector insert of traversing random order hash map.
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<decl_pair> 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
(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<decl_pair> 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.
(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
(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).
(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
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
(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.
Note that the patch was reverted in r216502.
GCC 4.9.2 has been released.
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 <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. 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
Fixed on the trunk so far.
Not a P1 for the branch, furthermore not easily backportable.
GCC 4.9.3 has been released.
Fixed.