Hi,
On Tue, Mar 01, 2016 at 09:04:25AM +1100, kugan wrote:
> Hi,
>
> As discussed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69708 and
> corresponding mailing list discussion, IPA CP is not detecting a
> jump-function with the sq function as value.
>
>
sorry it took so long for me to look at this. First, I have looked at
your patch and found a number of issues (see comments below), but when
I tried to fix them (see my patch below), I found out that using the
aggregate jump functions is not the the best approach. But let me
start with the comments first:
>
>
> 2016-03-01 Kugan Vivekanandarajah <kug...@linaro.org>
>
>
>
> * ipa-prop.c (determine_locally_known_aggregate_parts): Determine jump
>
> function for static constant initialization.
>
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 72c2fed..22da097 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -1562,6 +1562,57 @@ determine_locally_known_aggregate_parts (gcall *call,
> tree arg,
> jfunc->agg.by_ref = by_ref;
> build_agg_jump_func_from_list (list, const_count, arg_offset, jfunc);
> }
> + else if ((TREE_CODE (arg) == VAR_DECL)
> + && is_global_var (arg))
It would be better to check for this before iterating over statements
because they of course cannot write anything useful to a constant
global.
> + {
> + /* PR69708: Figure out aggregate jump-function with constant init
> + value. */
> + struct ipa_known_agg_contents_list *n, **p;
> + HOST_WIDE_INT offset = 0, size, max_size;
> + varpool_node *node = varpool_node::get (arg);
What do you need the varpool_node for? node->decl should always be
arg, I believe that unless you use ultimate_alias_target, node->decl
will always be arg.
> + if (node
> + && DECL_INITIAL (node->decl)
> + && TREE_READONLY (node->decl)
> + && TREE_CODE (DECL_INITIAL (node->decl)) == CONSTRUCTOR)
> + {
> + tree exp = DECL_INITIAL (node->decl);
> + unsigned HOST_WIDE_INT ix;
> + tree field, val;
> + bool reverse;
> + FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (exp), ix, field, val)
> + {
> + bool already_there = false;
> + if (!field)
> + break;
> + get_ref_base_and_extent (field, &offset, &size,
> + &max_size, &reverse);
I think you got this working just by luck. On your testcase, field is
a field_decl which get_ref_base_and_extent does not handle and so the
returned offset is always zero. Just add another field before the
call field and see for yourself.
Moreover, we should be able to also handle global read only arrays of
functions where field would be an index or NULL. And we should also
try to handle structures containing such arrays, which means
constructors would be nested (see the last testcase of the patch
below).
> + if (max_size == -1
> + || max_size != size)
> + break;
> + p = get_place_in_agg_contents_list (&list, offset, size,
> + &already_there);
> + if (!p)
> + break;
> + n = XALLOCA (struct ipa_known_agg_contents_list);
I believe the elements of a constructor are always already sorted and
so xalloca and insert-sort performed by get_place_in_agg_contents_list
is not necessary.
> + n->size = size;
> + n->offset = offset;
> + if (is_gimple_ip_invariant (val))
> + {
Nevertheless, thanks for the patch, as it shows how the issue can be
somewhat fixed fairly easily, even though imperfectly. I have fixed
the issues above and came up with the patch below. However, if you
look at the testcases you'll see xfails because even though we should
be able to find the target of all calls in foo and bar functions, in a
few testcases we only manage to find the first or none at all.
The first reason for that is that when we identify whether targets of
indirect calls come from a parameter, alias analysis tells us that the
first call might have changed the values in the aggregate and thus we
do not identify the subsequent calls as calling a target that we know
comes from a parameter.
To fix this, we must do two things. First, store the parameter index
and offset to indirect edges even if alias analysis tells us that the
value might be overwritten, which however must be stored there too, so
that we must use the index/offset information only when the we later
on discover that the parameter points to a constant variable. So this
needs to be marked somewhere in the jump function too. Thah alone
However, a similar AA issue is going to persist for pass-through jump
functions, as shown by xfailing ipcp-cstagg-7.c testcase. To fix
that, we'd either have to force propagation of aggregate values from
constant globals even through jump functions that have agg_preserved
flag cleared, or, and I think this is perhaps a better idea, rethink
the whole approach, give up creating aggregate jump functions and
instead use normal scalar propagation (even for non-scalar types, if
they are exact copies of a read-only aggregate) and change the
consumers so that they use the static initializers to look up the
value. This would also have the added advantage that parameter
PARAM_IPA_MAX_AGG_ITEMS would not be an issue.
So he current effort below is basically only for reference, hopefully
we'll be able to implement the second approach at some point during
stage1.
Thanks for raising this issue,
Martin
2016-03-10 Martin Jambor <mjam...@suse.cz>
* ipa-prop.c (count_constants_in_agg_constructor): New function.
(build_agg_jump_func_from_constructor): Likewise.
(determine_locally_known_aggregate_parts): Use thwem to process
global constant variables.
(parm_preserved_before_stmt_p): Return true for loads from
TREE_READONLY parameters.
testsuite/
* gcc.dg/ipa/ipcp-cstagg-1.c: New test.
* gcc.dg/ipa/ipcp-cstagg-2.c: Likewise.
* gcc.dg/ipa/ipcp-cstagg-3.c: Likewise.
* gcc.dg/ipa/ipcp-cstagg-4.c: Likewise.
* gcc.dg/ipa/ipcp-cstagg-5.c: Likewise.
* gcc.dg/ipa/ipcp-cstagg-6.c: Likewise.
* gcc.dg/ipa/ipcp-cstagg-7.c: Likewise.
---
gcc/ipa-prop.c | 141 +++++++++++++++++++++++++++++++
gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-1.c | 32 +++++++
gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c | 39 +++++++++
gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-3.c | 37 ++++++++
gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-4.c | 39 +++++++++
gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-5.c | 59 +++++++++++++
gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-6.c | 81 ++++++++++++++++++
gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-7.c | 46 ++++++++++
8 files changed, 474 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-1.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-3.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-4.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-5.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-6.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-7.c
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index d62c704..e0bc307 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -803,6 +803,11 @@ parm_preserved_before_stmt_p (struct ipa_func_body_info
*fbi, int index,
bool modified = false;
ao_ref refd;
+ tree base = get_base_address (parm_load);
+ gcc_assert (TREE_CODE (base) == PARM_DECL);
+ if (TREE_READONLY (base))
+ return true;
+
/* FIXME: FBI can be NULL if we are being called from outside
ipa_node_analysis or ipcp_transform_function, which currently happens
during inlining analysis. It would be great to extend fbi's lifetime and
@@ -1395,6 +1400,121 @@ build_agg_jump_func_from_list (struct
ipa_known_agg_contents_list *list,
}
}
+/* Return how many interprocedural scalar invariants there are in a static
+ CONSTRUCTOR of a variable. */
+
+static unsigned
+count_constants_in_agg_constructor (tree constructor)
+{
+ unsigned res = 0, max = PARAM_VALUE (PARAM_IPA_MAX_AGG_ITEMS);
+ unsigned ix;
+ tree index, val;
+ FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (constructor), ix, index, val)
+ {
+ if (TREE_CODE (TREE_TYPE (constructor)) == RECORD_TYPE
+ && !index)
+ /* We cannot handle field elements that do not have the field decl as
+ its index. */
+ continue;
+ if (is_gimple_reg_type (TREE_TYPE (val))
+ && is_gimple_ip_invariant (val))
+ res++;
+ else if (TREE_CODE (val) == CONSTRUCTOR)
+ res += count_constants_in_agg_constructor (val);
+
+ if (res > max)
+ return max;
+ }
+ return res;
+}
+
+/* Push invariants from static constructor of a global variable into JFUNC's
+ aggregate jump function. BASE_OFFSET is the offset which should be added to
+ offset of each value. It can be negative to represent that only a part of
+ an aggregate starting at BASE_OFFSET is being passed as an actual
+ argument. */
+
+static void
+build_agg_jump_func_from_constructor (tree constructor,
+ HOST_WIDE_INT base_offset,
+ struct ipa_jump_func *jfunc)
+{
+ tree type = TREE_TYPE (constructor);
+ if (TREE_CODE (type) != ARRAY_TYPE
+ && TREE_CODE (type) != RECORD_TYPE)
+ return;
+
+ unsigned ix;
+ tree index, val;
+ FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (constructor), ix, index, val)
+ {
+ if (!jfunc->agg.items->space (1))
+ return;
+
+ HOST_WIDE_INT elt_offset;
+ if (TREE_CODE (type) == ARRAY_TYPE)
+ {
+ offset_int off;
+ tree unit_size = TYPE_SIZE_UNIT (TREE_TYPE (type));
+ gcc_assert (TREE_CODE (unit_size) == INTEGER_CST);
+
+ if (index)
+ {
+ off = wi::to_offset (index);
+ if (TYPE_DOMAIN (type) && TYPE_MIN_VALUE (TYPE_DOMAIN (type)))
+ {
+ tree low_bound = TYPE_MIN_VALUE (TYPE_DOMAIN (type));
+ gcc_assert (TREE_CODE (unit_size) == INTEGER_CST);
+ off = wi::sext (off - wi::to_offset (low_bound),
+ TYPE_PRECISION (TREE_TYPE (index)));
+ }
+ off *= wi::to_offset (unit_size);
+ }
+ else
+ off = wi::to_offset (unit_size) * ix;
+
+ off = wi::lshift (off, LOG2_BITS_PER_UNIT);
+ if (!wi::fits_shwi_p (off) || wi::neg_p (off))
+ continue;
+ elt_offset = off.to_shwi ();
+ }
+ else if (TREE_CODE (type) == RECORD_TYPE)
+ {
+ gcc_checking_assert (index && TREE_CODE (index) == FIELD_DECL);
+ if (DECL_BIT_FIELD (index))
+ continue;
+ elt_offset = int_bit_position (index);
+ }
+ else
+ gcc_unreachable ();
+
+ if (is_gimple_reg_type (TREE_TYPE (val))
+ && is_gimple_ip_invariant (val))
+ {
+ HOST_WIDE_INT offset = elt_offset + base_offset;
+ if (offset >= 0
+ && (offset % BITS_PER_UNIT) == 0)
+ {
+ /* FIXME: The following check is just meant for developement, to
+ double check my assumption about constructor elements
+ ordering, either remove it or change it to a checking
+ assert. */
+ if (!jfunc->agg.items->is_empty ())
+ gcc_assert ((*jfunc->agg.items)[jfunc->agg.items->length () - 1]
+ .offset < offset);
+
+ struct ipa_agg_jf_item item;
+ item.offset = offset;
+ item.value = unshare_expr_without_location (val);
+ jfunc->agg.items->quick_push (item);
+ }
+ }
+ else if (TREE_CODE (val) == CONSTRUCTOR)
+ build_agg_jump_func_from_constructor (val, base_offset + elt_offset,
+ jfunc);
+ }
+}
+
/* Traverse statements from CALL backwards, scanning whether an aggregate given
in ARG is filled in with constant values. ARG can either be an aggregate
expression or a pointer to an aggregate. ARG_TYPE is the type of the
@@ -1475,6 +1595,27 @@ determine_locally_known_aggregate_parts (gcall *call,
tree arg,
ao_ref_init (&r, arg);
}
+ /* If we happen to be using a constant global variable or a reference to it,
+ construct the jump function from the static initializer. */
+
+ if ((TREE_CODE (arg) == VAR_DECL)
+ && is_global_var (arg)
+ && DECL_INITIAL (arg)
+ && TREE_READONLY (arg)
+ && TREE_CODE (DECL_INITIAL (arg)) == CONSTRUCTOR)
+ {
+ /* PR69708: Figure out aggregate jump-function with constant init
+ value. */
+ tree constructor = DECL_INITIAL (arg);
+ const_count = count_constants_in_agg_constructor (constructor);
+ if (!const_count)
+ return;
+ vec_alloc (jfunc->agg.items, const_count);
+ jfunc->agg.by_ref = by_ref;
+ build_agg_jump_func_from_constructor (constructor, -arg_offset, jfunc);
+ return;
+ }
+
/* Second stage walks back the BB, looks at individual statements and as long
as it is confident of how the statements affect contents of the
aggregates, it builds a sorted linked list of ipa_agg_jf_list structures
diff --git a/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-1.c
b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-1.c
new file mode 100644
index 0000000..ea7126b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-1.c
@@ -0,0 +1,32 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-ipa-cp-details" } */
+
+typedef struct S
+{
+ int (*call)(int);
+} S;
+
+static int __attribute__((noinline))
+foo (S f, int x)
+{
+ x = f.call(x);
+ x = f.call(x);
+ x = f.call(x);
+ return x;
+}
+
+static int
+sq (int x)
+{
+ return x * x;
+}
+
+static const S s = {sq};
+
+int
+h (int x)
+{
+ return foo (s, x);
+}
+
+/* { dg-final { scan-ipa-dump-times "Discovered an indirect call to a known
target" 3 "cp" } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c
b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c
new file mode 100644
index 0000000..d7899d9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c
@@ -0,0 +1,39 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-ipa-cp-details" } */
+
+typedef struct S
+{
+ int (*call)(int);
+} S;
+
+static int __attribute__((noinline))
+bar (const S *f, int x)
+{
+ x = f->call(x);
+ x = f->call(x);
+ x = f->call(x);
+ return x;
+}
+
+static int
+sq (int x)
+{
+ return x * x;
+}
+
+static const S s = {sq};
+
+int
+g (int x)
+{
+ return bar (&s, x);
+}
+
+int
+obfuscate (int x)
+{
+ return bar ((S *) 0, x);
+}
+
+/* { dg-final { scan-ipa-dump "Discovered an indirect call to a known target"
"cp" } } */
+/* { dg-final { scan-ipa-dump-times "Discovered an indirect call to a known
target" 3 "cp" { xfail *-*-* } } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-3.c
b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-3.c
new file mode 100644
index 0000000..4b27a27
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-3.c
@@ -0,0 +1,37 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-ipa-cp-details" } */
+
+typedef struct S
+{
+ int confuse;
+ int (*call)(int);
+} S;
+
+extern const S *es;
+
+static int __attribute__((noinline))
+foo (const S f, int x)
+{
+ es = &f; /* This disables IPA-SRA */
+ x = f.call(x+f.confuse);
+ x = f.call(x);
+ x = f.call(x);
+ return x;
+}
+
+static int
+sq (int x)
+{
+ return x * x;
+}
+
+static const S s = {16, sq};
+
+int
+h (int x)
+{
+ return foo (s, x);
+}
+
+/* { dg-final { scan-ipa-dump "Discovered an indirect call to a known target"
"cp" } } */
+/* { dg-final { scan-ipa-dump-times "Discovered an indirect call to a known
target" 3 "cp" } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-4.c
b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-4.c
new file mode 100644
index 0000000..d7899d9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-4.c
@@ -0,0 +1,39 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-ipa-cp-details" } */
+
+typedef struct S
+{
+ int (*call)(int);
+} S;
+
+static int __attribute__((noinline))
+bar (const S *f, int x)
+{
+ x = f->call(x);
+ x = f->call(x);
+ x = f->call(x);
+ return x;
+}
+
+static int
+sq (int x)
+{
+ return x * x;
+}
+
+static const S s = {sq};
+
+int
+g (int x)
+{
+ return bar (&s, x);
+}
+
+int
+obfuscate (int x)
+{
+ return bar ((S *) 0, x);
+}
+
+/* { dg-final { scan-ipa-dump "Discovered an indirect call to a known target"
"cp" } } */
+/* { dg-final { scan-ipa-dump-times "Discovered an indirect call to a known
target" 3 "cp" { xfail *-*-* } } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-5.c
b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-5.c
new file mode 100644
index 0000000..abf2f3d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-5.c
@@ -0,0 +1,59 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-ipa-cp-details" } */
+
+#define N 4
+
+typedef int (* const A[N])(int);
+
+extern const A *ga;
+
+static int __attribute__((noinline))
+bar (const A *f, int x)
+{
+ x = (*f)[2](x);
+ x = (*f)[2](x);
+ x = (*f)[2](x);
+ ga = f;
+ return x;
+}
+
+static int
+zero (int x)
+{
+ return 0;
+}
+
+static int
+addone (int x)
+{
+ return x + 1;
+}
+
+static int
+sq (int x)
+{
+ return x * x;
+}
+
+static int
+cube (int x)
+{
+ return x * x * x;
+}
+
+static const A a = {zero, addone, sq, cube};
+
+int
+g (int x)
+{
+ return bar (&a, x);
+}
+
+int
+obfuscate (int x)
+{
+ return bar ((A *) 0, x);
+}
+
+/* { dg-final { scan-ipa-dump "Discovered an indirect call to a known target"
"cp" } } */
+/* { dg-final { scan-ipa-dump-times "Discovered an indirect call to a known
target" 3 "cp" { xfail *-*-* } } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-6.c
b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-6.c
new file mode 100644
index 0000000..d19a601
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-6.c
@@ -0,0 +1,81 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-ipa-cp-details" } */
+
+#define N 4
+
+typedef int (* const A[N])(int);
+
+typedef struct S
+{
+ int obfuscate;
+ A a;
+} S;
+
+extern const S *gs;
+
+static int __attribute__((noinline))
+foo (const S f, int x)
+{
+ gs = &f;
+ x = f.a[2](x);
+ x = f.a[2](x);
+ x = f.a[2](x);
+ return x;
+}
+
+static int __attribute__((noinline))
+bar (const S *f, int x)
+{
+ gs = f;
+ x = f->a[2](x);
+ x = f->a[2](x);
+ x = f->a[2](x);
+ return x;
+}
+
+static int
+zero (int x)
+{
+ return 0;
+}
+
+static int
+addone (int x)
+{
+ return x + 1;
+}
+
+static int
+sq (int x)
+{
+ return x * x;
+}
+
+static int
+cube (int x)
+{
+ return x * x * x;
+}
+
+static const S s = {64, {zero, addone, sq, cube}};
+
+int
+h (int x)
+{
+ return foo (s, x);
+}
+
+int
+g (int x)
+{
+ return bar (&s, x);
+}
+
+int
+obfuscate (int x)
+{
+ return bar ((S *) 0, x);
+}
+
+/* { dg-final { scan-ipa-dump "Discovered an indirect call to a known target"
"cp" } } */
+/* { dg-final { scan-ipa-dump-times "Discovered an indirect call to a known
target" 6 "cp" { xfail *-*-* } } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-7.c
b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-7.c
new file mode 100644
index 0000000..19daefc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-7.c
@@ -0,0 +1,46 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -fdump-ipa-cp-details" } */
+
+typedef struct S
+{
+ int (*call)(int);
+} S;
+
+static int __attribute__((noinline))
+bar (const S *f, int x)
+{
+ x = f->call(x);
+ return x;
+}
+
+extern void impossible_aa (void);
+
+static int __attribute__((noinline))
+baz (const S *f, int x)
+{
+ impossible_aa ();
+ return bar (f, x);
+}
+
+static int
+sq (int x)
+{
+ return x * x;
+}
+
+static const S s = {sq};
+
+int
+g (int x)
+{
+ return baz (&s, x);
+}
+
+int
+obfuscate (int x)
+{
+ return baz ((S *) 0, x);
+}
+
+/* { dg-final { scan-ipa-dump "Discovered an indirect call to a known target"
"cp" { xfail *-*-* } } } */
+
--
2.7.1
