On 03/15/2016 07:13 AM, Richard Biener wrote:
On Tue, Mar 15, 2016 at 4:44 AM, Richard Henderson <r...@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.
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
diff --git a/gcc/function.h b/gcc/function.h
index c4368cd..501ef68 100644
--- a/gcc/function.h
+++ b/gcc/function.h
@@ -328,6 +328,10 @@ struct GTY(()) function {
from nested functions. */
unsigned int has_nonlocal_label : 1;
+ /* Nonzero if function being compiled has a forced label
+ placed into static storage. */
+ unsigned int has_forced_label_in_static : 1;
+
/* Nonzero if we've set cannot_be_copied_reason. I.e. if
(cannot_be_copied_set && !cannot_be_copied_reason), the function
can in fact be copied. */
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index b331e41..c60ac13 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -1414,7 +1414,10 @@ force_labels_r (tree *tp, int *walk_subtrees, void *data
ATTRIBUTE_UNUSED)
if (TYPE_P (*tp))
*walk_subtrees = 0;
if (TREE_CODE (*tp) == LABEL_DECL)
- FORCED_LABEL (*tp) = 1;
+ {
+ FORCED_LABEL (*tp) = 1;
+ cfun->has_forced_label_in_static = 1;
+ }
return NULL_TREE;
}
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..65ec2f9 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -3549,19 +3549,12 @@ copy_forbidden (struct function *fun, tree fndecl)
goto fail;
}
- FOR_EACH_LOCAL_DECL (fun, ix, decl)
- if (TREE_CODE (decl) == VAR_DECL
- && TREE_STATIC (decl)
- && !DECL_EXTERNAL (decl)
- && DECL_INITIAL (decl)
- && walk_tree_without_duplicates (&DECL_INITIAL (decl),
- has_label_address_in_static_1,
- fndecl))
- {
- reason = G_("function %q+F can never be copied because it saves "
- "address of local label in a static variable");
- goto fail;
- }
+ if (fun->has_forced_label_in_static)
+ {
+ reason = G_("function %q+F can never be copied because it saves "
+ "address of local label in a static variable");
+ goto fail;
+ }
fail:
fun->cannot_be_copied_reason = reason;
