| Message ID | 56E86591.1040904@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 15, 2016 at 8:42 PM, Richard Henderson <rth@redhat.com> wrote: > On 03/15/2016 07:13 AM, Richard Biener wrote: >> >> On Tue, Mar 15, 2016 at 4:44 AM, Richard Henderson <rth@redhat.com> wrote: >>> >>> The problem here is that >>> >>> void* labels[] = { >>> &&l0, &&l1, &&l2 >>> }; >>> >>> gets gimplified to >>> >>> labels = *.LC0; >>> >>> but .LC0 is not in the set of local decls, so that when copy_forbidden is >>> called during sra versioning we fail to forbid the copy. We could set a >>> different flag, but I think it's easiest to just add the artificial decl >>> to >>> where it can be seen. >>> >>> Ok? >> >> >> Hmm. tree_output_constant_def uses the global constant pool (and not >> function-scope statics). So while for the above case with local labels >> there can be no sharing and thus the decl is really "local" with non-local >> labels or with other random initializers you'd have the ctor decl in >> multiple local decl vectors. Not sure if that's a problem, but at least >> if you'd have >> >> void* labels[] = { >> &&l0, &&l1, &&l2 >> }; >> void* labels2[] = { >> &&l0, &&l1, &&l2 >> }; >> >> you'll end up with the same constant pool decl in local-decls twice. > > > Yeah, but since the decl is TREE_STATIC, we'll ignore it for almost > everything. About the only thing I can figure that might go wrong is unused > variable removal, where we'd remove the first copy but not look for > duplicates, and so the variable stays in use when it isn't. I don't *think* > that can cause further problems. It's not like we ever clear FORCED_LABEL > even if the data referencing it goes away. > >> It's also >> a bit pre-mature in the gimplifier as we only add to local-decls during >> BIND expr lowering. > > > Yeah, I suppose. Though for a TREE_STATIC decl it doesn't make a difference > that we didn't put it into any BIND_EXPR. > >> I also wonder if outputting the constant pool decl far away from the >> labels >> might end up with invalid asm for some targets. > > > No. The pointers involved here are full address space, not reduced > displacement pc-relative. > >> Well, I don't see any convenient way of fixing things here either but >> maybe >> we can do >> >> if (walk_tree_without_duplicataes (&DECL_INITIAL (ctor), >> has_label_address_in_static_1, cfun->decl)) >> add_local_decl (cfun, ctor); >> >> to avoid adding the decl when it is not necessary. > > > Sure. Patch 1 below. > >> Having another struct function flag would be possible as well, or re-use >> has_nonlocal_label as clearly a global static is now refering to a local >> label (you'd lose optimization when 'labels' becomes unused of course). > > > On the other hand, the likelyhood of these labels (or the data referencing > the labels) going away is slim. Except for artificial test cases, the user > is going to have taken these addresses and put them in an array for a > reason. The likelyhood of some stored FORCED_LABEL becoming non-forced is > virtually nil. > > Patch 2 below. This second patch does have lower complexity, and doesn't > have the duplicated entry issue you point out. I like patch 2 more - btw, you need to add has_forced_label_in_static streaming to lto-streamer-{in,out}.c, just look for has_nonlocal_label streaming. Also has_label_address_in_static_1 is now unused and should be removed. Thanks, Richard. > Thoughts? > > > r~
diff --git a/gcc/gimplify.c b/gcc/gimplify.c index b331e41..cf50271 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -4016,6 +4016,14 @@ gimplify_init_constructor (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, walk_tree (&ctor, force_labels_r, NULL, NULL); ctor = tree_output_constant_def (ctor); + + /* If the ctor has a label in it, we need to remember the + decl so that copy_forbidden can find it. But for anything + else we don't want to place the global variable on the + local decls list. */ + if (has_label_address_in_static (ctor, cfun->decl)) + add_local_decl (cfun, ctor); + if (!useless_type_conversion_p (type, TREE_TYPE (ctor))) ctor = build1 (VIEW_CONVERT_EXPR, type, ctor); TREE_OPERAND (*expr_p, 1) = ctor; diff --git a/gcc/testsuite/gcc.c-torture/compile/pr70199.c b/gcc/testsuite/gcc.c-torture/compile/pr70199.c new file mode 100644 index 0000000..a4323f0 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr70199.c @@ -0,0 +1,20 @@ +static volatile int v = 0; +static +void benchmark(long runs) { + void* labels[] = { + &&l0, &&l1, &&l2 + }; + for(unsigned int mask = 0x1F; mask > 0; mask >>= 1) { + unsigned lfsr = 0xACE1u; + long n = 10000000; + while(n > 0) { + l2: v; + l1: v; + goto *labels[lfsr & mask]; + l0: n--; + } + } +} +int f(void) { + benchmark(10000000); +} diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index d52e0c6..cac2340 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -3522,6 +3522,17 @@ has_label_address_in_static_1 (tree *nodep, int *walk_subtrees, void *fnp) return NULL_TREE; } +/* Determine if the DECL_INITIAL of DECL makes a reference to a label that + is local to FNDECL. */ + +bool +has_label_address_in_static (tree decl, tree fndecl) +{ + return walk_tree_without_duplicates (&DECL_INITIAL (decl), + has_label_address_in_static_1, + fndecl) != NULL_TREE; +} + /* Determine if the function can be copied. If so return NULL. If not return a string describng the reason for failure. */ @@ -3554,9 +3565,7 @@ copy_forbidden (struct function *fun, tree fndecl) && TREE_STATIC (decl) && !DECL_EXTERNAL (decl) && DECL_INITIAL (decl) - && walk_tree_without_duplicates (&DECL_INITIAL (decl), - has_label_address_in_static_1, - fndecl)) + && has_label_address_in_static (decl, fndecl)) { reason = G_("function %q+F can never be copied because it saves " "address of local label in a static variable"); diff --git a/gcc/tree-inline.h b/gcc/tree-inline.h index 4cc1f19..3b01091 100644 --- a/gcc/tree-inline.h +++ b/gcc/tree-inline.h @@ -217,6 +217,7 @@ extern tree remap_type (tree type, copy_body_data *id); extern gimple_seq copy_gimple_seq_and_replace_locals (gimple_seq seq); extern bool debug_find_tree (tree, tree); extern tree copy_fn (tree, tree&, tree&); +extern bool has_label_address_in_static (tree decl, tree fndecl); extern const char *copy_forbidden (struct function *fun, tree fndecl); /* This is in tree-inline.c since the routine uses