Re: [RFC][PR69708] IPA inline not working for function reference in static const struc

[kugan]

On 11/03/16 03:39, Martin Jambor wrote:

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:


Hi Martin,

Thanks for the explanation.

Kugan

Re: [RFC][PR69708] IPA inline not working for function reference in static const struc

[Martin Jambor]

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  
> 
> 
> 
>   * 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, , ,
> +_size, );

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 (, offset, size,
> +   _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 

[RFC][PR69708] IPA inline not working for function reference in static const struc

[kugan]

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.



static int sq(int x) {
  return x * x;
}

static const F f = {sq};
...
dosomething (g(f, x));
...

I added a check at  determine_locally_known_aggregate_parts to detect 
this. This fixes the testcase and passes x86-64-linux-gnu lto bootstrap 
and regression testing with no new regression. Does this look sensible 
place to fix this?


Thanks,
Kugan

gcc/ChangeLog:



2016-03-01  Kugan Vivekanandarajah  



* 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))
+{
+  /* 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);
+  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, , ,
+  _size, );
+ if (max_size == -1
+ || max_size != size)
+   break;
+ p = get_place_in_agg_contents_list (, offset, size,
+ _there);
+ if (!p)
+   break;
+ n = XALLOCA (struct ipa_known_agg_contents_list);
+ n->size = size;
+ n->offset = offset;
+ if (is_gimple_ip_invariant (val))
+   {
+ n->constant = val;
+ const_count++;
+   }
+ else
+   n->constant = NULL_TREE;
+ n->next = *p;
+ *p = n;
+   }
+   }
+  if (const_count)
+   {
+ jfunc->agg.by_ref = by_ref;
+ build_agg_jump_func_from_list (list, const_count, arg_offset, jfunc);
+   }
+}
 }
 
 static tree