[PATCH] Fix PR66705

[PATCH] Fix PR66705

[Richard Biener]

I was naiively using ->get_constructor in IPA PTA without proper
checking on wheter that succeeds.  Now I tried to use ctor_for_folding
but that isn't good as we want to analyze non-const globals in IPA
PTA and we need to analyze their initialiers as well.

Thus I'm trying below with ctor_for_analysis, but I really "just"
need the initializer or a "not available" for conservative handling.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

Honza - I suppose you should doble-check this and suggest sth
different (or implement sth more generic in the IPA infrastructure).

Thanks,
Richard.

2015-09-02  Richard Biener  <rguenther@suse.de>

	PR ipa/66705
	* tree-ssa-structalias.c (ctor_for_analysis): New function.
	(create_variable_info_for_1): Use ctor_for_analysis instead
	of get_constructor.
	(create_variable_info_for): Likewise.

	* g++.dg/lto/pr66705_0.C: New testcase.

Index: gcc/tree-ssa-structalias.c
===================================================================
--- gcc/tree-ssa-structalias.c	(revision 227207)
+++ gcc/tree-ssa-structalias.c	(working copy)
@@ -5637,6 +5637,26 @@ check_for_overlaps (vec<fieldoff_s> fiel
   return false;
 }
 
+/* We can't use ctor_for_folding as that only returns constant constructors.  */
+
+static tree
+ctor_for_analysis (tree decl)
+{
+  varpool_node *node = varpool_node::get (decl);
+  if (!node)
+    return error_mark_node;
+  node = node->ultimate_alias_target ();
+  if (DECL_INITIAL (node->decl) != error_mark_node
+      || !in_lto_p)
+    return (DECL_INITIAL (node->decl)
+	    ? DECL_INITIAL (node->decl) : error_mark_node);
+  if (in_lto_p
+      && node->lto_file_data
+      && !node->body_removed)
+    return node->get_constructor ();
+  return error_mark_node;
+}
+
 /* Create a varinfo structure for NAME and DECL, and add it to VARMAP.
    This will also create any varinfo structures necessary for fields
    of DECL.  */
@@ -5650,7 +5670,6 @@ create_variable_info_for_1 (tree decl, c
   auto_vec<fieldoff_s> fieldstack;
   fieldoff_s *fo;
   unsigned int i;
-  varpool_node *vnode;
 
   if (!declsize
       || !tree_fits_uhwi_p (declsize))
@@ -5672,8 +5691,7 @@ create_variable_info_for_1 (tree decl, c
 	 in IPA mode.  Else we'd have to parse arbitrary initializers.  */
       && !(in_ipa_mode
 	   && is_global_var (decl)
-	   && (vnode = varpool_node::get (decl))
-	   && vnode->get_constructor ()))
+	   && ctor_for_analysis (decl) != error_mark_node))
     {
       fieldoff_s *fo = NULL;
       bool notokay = false;
@@ -5805,13 +5823,13 @@ create_variable_info_for (tree decl, con
 
 	  /* If this is a global variable with an initializer and we are in
 	     IPA mode generate constraints for it.  */
-	  if (vnode->get_constructor ()
-	      && vnode->definition)
+	  tree ctor = ctor_for_analysis (decl);
+	  if (ctor != error_mark_node)
 	    {
 	      auto_vec<ce_s> rhsc;
 	      struct constraint_expr lhs, *rhsp;
 	      unsigned i;
-	      get_constraint_for_rhs (vnode->get_constructor (), &rhsc);
+	      get_constraint_for_rhs (ctor, &rhsc);
 	      lhs.var = vi->id;
 	      lhs.offset = 0;
 	      lhs.type = SCALAR;
Index: gcc/testsuite/g++.dg/lto/pr66705_0.C
===================================================================
--- gcc/testsuite/g++.dg/lto/pr66705_0.C	(revision 0)
+++ gcc/testsuite/g++.dg/lto/pr66705_0.C	(working copy)
@@ -0,0 +1,15 @@
+// { dg-lto-do link }
+// { dg-lto-options { { -O2 -flto -flto-partition=max -fipa-pta } } }
+// { dg-extra-ld-options "-r -nostdlib" }
+
+class A {
+public:
+    A();
+};
+int a = 0;
+void foo() {
+    a = 0;
+    A b;
+    for (; a;)
+      ;
+}

Re: [PATCH] Fix PR66705

[Jan Hubicka]

> 
> I was naiively using ->get_constructor in IPA PTA without proper
> checking on wheter that succeeds.  Now I tried to use ctor_for_folding
> but that isn't good as we want to analyze non-const globals in IPA
> PTA and we need to analyze their initialiers as well.
> 
> Thus I'm trying below with ctor_for_analysis, but I really "just"
> need the initializer or a "not available" for conservative handling.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
> 
> Honza - I suppose you should doble-check this and suggest sth
> different (or implement sth more generic in the IPA infrastructure).

Yep, you are correct that we don't currently have way to look into ctor
without actually loading. But do you need something more than just walking
references that you already have in ipa-ref lists?
> 
> Thanks,
> Richard.
> 
> 2015-09-02  Richard Biener  <rguenther@suse.de>
> 
> 	PR ipa/66705
> 	* tree-ssa-structalias.c (ctor_for_analysis): New function.
> 	(create_variable_info_for_1): Use ctor_for_analysis instead
> 	of get_constructor.
> 	(create_variable_info_for): Likewise.

Otherwise I would go for making ctor_for_analysis a method of varpool_node and...
> 
> 	* g++.dg/lto/pr66705_0.C: New testcase.
> 
> Index: gcc/tree-ssa-structalias.c
> ===================================================================
> --- gcc/tree-ssa-structalias.c	(revision 227207)
> +++ gcc/tree-ssa-structalias.c	(working copy)
> @@ -5637,6 +5637,26 @@ check_for_overlaps (vec<fieldoff_s> fiel
>    return false;
>  }
>  
> +/* We can't use ctor_for_folding as that only returns constant constructors.  */
> +
> +static tree
> +ctor_for_analysis (tree decl)
> +{
> +  varpool_node *node = varpool_node::get (decl);
> +  if (!node)
> +    return error_mark_node;
> +  node = node->ultimate_alias_target ();
> +  if (DECL_INITIAL (node->decl) != error_mark_node
> +      || !in_lto_p)
> +    return (DECL_INITIAL (node->decl)
> +	    ? DECL_INITIAL (node->decl) : error_mark_node);

I think returning NULL here is just fine. 
error_mark_node means constructor is not really available. NULL is
the usual way to say that the variable is not initialized.

Re: [PATCH] Fix PR66705

[Richard Biener]

On Wed, 2 Sep 2015, Jan Hubicka wrote:

> > 
> > I was naiively using ->get_constructor in IPA PTA without proper
> > checking on wheter that succeeds.  Now I tried to use ctor_for_folding
> > but that isn't good as we want to analyze non-const globals in IPA
> > PTA and we need to analyze their initialiers as well.
> > 
> > Thus I'm trying below with ctor_for_analysis, but I really "just"
> > need the initializer or a "not available" for conservative handling.
> > 
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> > 
> > Honza - I suppose you should doble-check this and suggest sth
> > different (or implement sth more generic in the IPA infrastructure).
> 
> Yep, you are correct that we don't currently have way to look into ctor
> without actually loading. But do you need something more than just walking
> references that you already have in ipa-ref lists?

Hmm, no, ipa-ref list should be enough (unless we start field-sensitive
analysis or need NULL inits for correctness).  Still have to figure out
how to walk the list and how the reference would look like (what
is ref->use?  IPA_REF_ADDR?  can those be speculative?)

Richard.

> > 
> > Thanks,
> > Richard.
> > 
> > 2015-09-02  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR ipa/66705
> > 	* tree-ssa-structalias.c (ctor_for_analysis): New function.
> > 	(create_variable_info_for_1): Use ctor_for_analysis instead
> > 	of get_constructor.
> > 	(create_variable_info_for): Likewise.
> 
> Otherwise I would go for making ctor_for_analysis a method of varpool_node and...
> > 
> > 	* g++.dg/lto/pr66705_0.C: New testcase.
> > 
> > Index: gcc/tree-ssa-structalias.c
> > ===================================================================
> > --- gcc/tree-ssa-structalias.c	(revision 227207)
> > +++ gcc/tree-ssa-structalias.c	(working copy)
> > @@ -5637,6 +5637,26 @@ check_for_overlaps (vec<fieldoff_s> fiel
> >    return false;
> >  }
> >  
> > +/* We can't use ctor_for_folding as that only returns constant constructors.  */
> > +
> > +static tree
> > +ctor_for_analysis (tree decl)
> > +{
> > +  varpool_node *node = varpool_node::get (decl);
> > +  if (!node)
> > +    return error_mark_node;
> > +  node = node->ultimate_alias_target ();
> > +  if (DECL_INITIAL (node->decl) != error_mark_node
> > +      || !in_lto_p)
> > +    return (DECL_INITIAL (node->decl)
> > +	    ? DECL_INITIAL (node->decl) : error_mark_node);
> 
> I think returning NULL here is just fine. 
> error_mark_node means constructor is not really available. NULL is
> the usual way to say that the variable is not initialized.
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix PR66705

[Richard Biener]

On Wed, 2 Sep 2015, Richard Biener wrote:

> On Wed, 2 Sep 2015, Jan Hubicka wrote:
> 
> > > 
> > > I was naiively using ->get_constructor in IPA PTA without proper
> > > checking on wheter that succeeds.  Now I tried to use ctor_for_folding
> > > but that isn't good as we want to analyze non-const globals in IPA
> > > PTA and we need to analyze their initialiers as well.
> > > 
> > > Thus I'm trying below with ctor_for_analysis, but I really "just"
> > > need the initializer or a "not available" for conservative handling.
> > > 
> > > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> > > 
> > > Honza - I suppose you should doble-check this and suggest sth
> > > different (or implement sth more generic in the IPA infrastructure).
> > 
> > Yep, you are correct that we don't currently have way to look into ctor
> > without actually loading. But do you need something more than just walking
> > references that you already have in ipa-ref lists?
> 
> Hmm, no, ipa-ref list should be enough (unless we start field-sensitive
> analysis or need NULL inits for correctness).  Still have to figure out
> how to walk the list and how the reference would look like (what
> is ref->use?  IPA_REF_ADDR?  can those be speculative?)

Sth like the following seems to work.

Richard.

2015-09-02  Richard Biener  <rguenther@suse.de>

	PR ipa/66705
	* tree-ssa-structalias.c (ctor_for_analysis): New function.
	(create_variable_info_for_1): Use ctor_for_analysis instead
	of get_constructor.
	(create_variable_info_for): Likewise.

	* g++.dg/lto/pr66705_0.C: New testcase.

Index: gcc/tree-ssa-structalias.c
===================================================================
--- gcc/tree-ssa-structalias.c	(revision 227207)
+++ gcc/tree-ssa-structalias.c	(working copy)
@@ -5650,7 +5650,6 @@ create_variable_info_for_1 (tree decl, c
   auto_vec<fieldoff_s> fieldstack;
   fieldoff_s *fo;
   unsigned int i;
-  varpool_node *vnode;
 
   if (!declsize
       || !tree_fits_uhwi_p (declsize))
@@ -5668,12 +5667,10 @@ create_variable_info_for_1 (tree decl, c
   /* Collect field information.  */
   if (use_field_sensitive
       && var_can_have_subvars (decl)
-      /* ???  Force us to not use subfields for global initializers
-	 in IPA mode.  Else we'd have to parse arbitrary initializers.  */
+      /* ???  Force us to not use subfields for globals in IPA mode.
+	 Else we'd have to parse arbitrary initializers.  */
       && !(in_ipa_mode
-	   && is_global_var (decl)
-	   && (vnode = varpool_node::get (decl))
-	   && vnode->get_constructor ()))
+	   && is_global_var (decl)))
     {
       fieldoff_s *fo = NULL;
       bool notokay = false;
@@ -5805,13 +5802,13 @@ create_variable_info_for (tree decl, con
 
 	  /* If this is a global variable with an initializer and we are in
 	     IPA mode generate constraints for it.  */
-	  if (vnode->get_constructor ()
-	      && vnode->definition)
+	  ipa_ref *ref;
+	  for (unsigned idx = 0; vnode->iterate_reference (idx, ref); ++idx)
 	    {
 	      auto_vec<ce_s> rhsc;
 	      struct constraint_expr lhs, *rhsp;
 	      unsigned i;
-	      get_constraint_for_rhs (vnode->get_constructor (), &rhsc);
+	      get_constraint_for_address_of (ref->referred->decl, &rhsc);
 	      lhs.var = vi->id;
 	      lhs.offset = 0;
 	      lhs.type = SCALAR;
Index: gcc/testsuite/g++.dg/lto/pr66705_0.C
===================================================================
--- gcc/testsuite/g++.dg/lto/pr66705_0.C	(revision 0)
+++ gcc/testsuite/g++.dg/lto/pr66705_0.C	(working copy)
@@ -0,0 +1,15 @@
+// { dg-lto-do link }
+// { dg-lto-options { { -O2 -flto -flto-partition=max -fipa-pta } } }
+// { dg-extra-ld-options "-r -nostdlib" }
+
+class A {
+public:
+    A();
+};
+int a = 0;
+void foo() {
+    a = 0;
+    A b;
+    for (; a;)
+      ;
+}

Re: [PATCH] Fix PR66705

[Jan Hubicka]

> On Wed, 2 Sep 2015, Richard Biener wrote:
> 
> > On Wed, 2 Sep 2015, Jan Hubicka wrote:
> > 
> > > > 
> > > > I was naiively using ->get_constructor in IPA PTA without proper
> > > > checking on wheter that succeeds.  Now I tried to use ctor_for_folding
> > > > but that isn't good as we want to analyze non-const globals in IPA
> > > > PTA and we need to analyze their initialiers as well.
> > > > 
> > > > Thus I'm trying below with ctor_for_analysis, but I really "just"
> > > > need the initializer or a "not available" for conservative handling.
> > > > 
> > > > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> > > > 
> > > > Honza - I suppose you should doble-check this and suggest sth
> > > > different (or implement sth more generic in the IPA infrastructure).
> > > 
> > > Yep, you are correct that we don't currently have way to look into ctor
> > > without actually loading. But do you need something more than just walking
> > > references that you already have in ipa-ref lists?
> > 
> > Hmm, no, ipa-ref list should be enough (unless we start field-sensitive
> > analysis or need NULL inits for correctness).  Still have to figure out
> > how to walk the list and how the reference would look like (what
> > is ref->use?  IPA_REF_ADDR?  can those be speculative?)
> 
> Sth like the following seems to work.

Yep, it looks good to me. Do you conservatively handle constructors that are in other units?
Those won't have ipa-ref lists streamed to ltrans stage.  I suppose you do not care because
all references in them can be supplied by foreign code, so you need to be conservative anyway.

Honza

Re: [PATCH] Fix PR66705

[Jan Hubicka]

> 
> Hmm, no, ipa-ref list should be enough (unless we start field-sensitive
> analysis or need NULL inits for correctness).  Still have to figure out
> how to walk the list and how the reference would look like (what
> is ref->use?  IPA_REF_ADDR?  can those be speculative?)

Yep, it should be IPA_REF_ADDR.  If you do not check for that, you will
probably also trip IPA_REF_ALIAS as dereference (which would lead to missed
optimization).  If we go field sensitive, we may want to extend ipa references
anyway - that would be useful for ipa-reference and other stuff, too.

Honza
> 
> Richard.
> 
> > > 
> > > Thanks,
> > > Richard.
> > > 
> > > 2015-09-02  Richard Biener  <rguenther@suse.de>
> > > 
> > > 	PR ipa/66705
> > > 	* tree-ssa-structalias.c (ctor_for_analysis): New function.
> > > 	(create_variable_info_for_1): Use ctor_for_analysis instead
> > > 	of get_constructor.
> > > 	(create_variable_info_for): Likewise.
> > 
> > Otherwise I would go for making ctor_for_analysis a method of varpool_node and...
> > > 
> > > 	* g++.dg/lto/pr66705_0.C: New testcase.
> > > 
> > > Index: gcc/tree-ssa-structalias.c
> > > ===================================================================
> > > --- gcc/tree-ssa-structalias.c	(revision 227207)
> > > +++ gcc/tree-ssa-structalias.c	(working copy)
> > > @@ -5637,6 +5637,26 @@ check_for_overlaps (vec<fieldoff_s> fiel
> > >    return false;
> > >  }
> > >  
> > > +/* We can't use ctor_for_folding as that only returns constant constructors.  */
> > > +
> > > +static tree
> > > +ctor_for_analysis (tree decl)
> > > +{
> > > +  varpool_node *node = varpool_node::get (decl);
> > > +  if (!node)
> > > +    return error_mark_node;
> > > +  node = node->ultimate_alias_target ();
> > > +  if (DECL_INITIAL (node->decl) != error_mark_node
> > > +      || !in_lto_p)
> > > +    return (DECL_INITIAL (node->decl)
> > > +	    ? DECL_INITIAL (node->decl) : error_mark_node);
> > 
> > I think returning NULL here is just fine. 
> > error_mark_node means constructor is not really available. NULL is
> > the usual way to say that the variable is not initialized.
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix PR66705

[Richard Biener]

On Wed, 2 Sep 2015, Jan Hubicka wrote:

> > On Wed, 2 Sep 2015, Richard Biener wrote:
> > 
> > > On Wed, 2 Sep 2015, Jan Hubicka wrote:
> > > 
> > > > > 
> > > > > I was naiively using ->get_constructor in IPA PTA without proper
> > > > > checking on wheter that succeeds.  Now I tried to use ctor_for_folding
> > > > > but that isn't good as we want to analyze non-const globals in IPA
> > > > > PTA and we need to analyze their initialiers as well.
> > > > > 
> > > > > Thus I'm trying below with ctor_for_analysis, but I really "just"
> > > > > need the initializer or a "not available" for conservative handling.
> > > > > 
> > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> > > > > 
> > > > > Honza - I suppose you should doble-check this and suggest sth
> > > > > different (or implement sth more generic in the IPA infrastructure).
> > > > 
> > > > Yep, you are correct that we don't currently have way to look into ctor
> > > > without actually loading. But do you need something more than just walking
> > > > references that you already have in ipa-ref lists?
> > > 
> > > Hmm, no, ipa-ref list should be enough (unless we start field-sensitive
> > > analysis or need NULL inits for correctness).  Still have to figure out
> > > how to walk the list and how the reference would look like (what
> > > is ref->use?  IPA_REF_ADDR?  can those be speculative?)
> > 
> > Sth like the following seems to work.
> 
> Yep, it looks good to me. Do you conservatively handle constructors that 
> are in other units? Those won't have ipa-ref lists streamed to ltrans 
> stage.  I suppose you do not care because all references in them can be 
> supplied by foreign code, so you need to be conservative anyway.

I use all_refs_explicit_p () to go a conservative path.  And indeed
I may trip aliases (well, the code doesn't handle aliases correctly
anyway I guess - I just walk vars via

  /* Create constraints for global variables and their initializers.  */
  FOR_EACH_VARIABLE (var)
    {
      if (var->alias && var->analyzed)
        continue;

      get_vi_for_tree (var->decl);
    }

and in get_vi_for_tree look at its ref list.  So I should only get
"ultimate" alias targets and only those may have initializers?

Richard.

Re: [PATCH] Fix PR66705

[Jan Hubicka]

> > supplied by foreign code, so you need to be conservative anyway.
> 
> I use all_refs_explicit_p () to go a conservative path.  And indeed

Yep, that should be safe.

I guess you want two things
 1) discover any possible ADDR_REF of a given symbol with all_refs_explicit_p.
    For that you want walk referring list of the symbol or if you go forward
    direction walk all initializers of all non-aliases you can see as all of them
    may possibly reffer to it.
 2) discover what static variables you have fully in control and know all
    references to.  I suppose that is where you use all_refs_explicit_p
    Explicit reference may be also an alias that may be exported.
    all_refs_explicit_p cares about symbol, not about the initializer itself,
    so you may want to have a version of this predicate that walks all aliases
    and checks that all of them have all refs explicit.

> I may trip aliases (well, the code doesn't handle aliases correctly
> anyway I guess - I just walk vars via
> 
>   /* Create constraints for global variables and their initializers.  */
>   FOR_EACH_VARIABLE (var)
>     {
>       if (var->alias && var->analyzed)
>         continue;
> 
>       get_vi_for_tree (var->decl);
>     }
> 
> and in get_vi_for_tree look at its ref list.  So I should only get
> "ultimate" alias targets and only those may have initializers?

Yep, only ulitmate alias targets have initializers.

Honza
> 
> Richard.