diff mbox

Fix IPA CP where it forgot to add a reference in cgraph

Message ID 87b66126-34a7-4935-d078-a14d979b3c9a@suse.cz
State New
Headers show

Commit Message

Martin Liška Dec. 20, 2016, 2:55 p.m. UTC 
On 12/20/2016 11:06 AM, Martin Jambor wrote:
> ...this test should be for ADDR_EXPR here.  Or you could switch the

> IPA_REF_* constants the other way round which I bet is going to have

> the same effect in practice, but personally, I'd test for ADDR_EXPR.


Thanks for the note, fixed (and tested in second version of the patch).

Martin

> 

> Thanks,

> 

> Martin

Comments

Martin Liška Jan. 10, 2017, 10:49 a.m. UTC | #1 
PING^1

On 12/20/2016 03:55 PM, Martin Liška wrote:
> On 12/20/2016 11:06 AM, Martin Jambor wrote:

>> ...this test should be for ADDR_EXPR here.  Or you could switch the

>> IPA_REF_* constants the other way round which I bet is going to have

>> the same effect in practice, but personally, I'd test for ADDR_EXPR.

>

> Thanks for the note, fixed (and tested in second version of the patch).

>

> Martin

>

>>

>> Thanks,

>>

>> Martin

>
Jan Hubicka Jan. 18, 2017, 10:18 p.m. UTC | #2 
> 

> 2016-12-19  Martin Liska  <mliska@suse.cz>

> 

> 	* cgraphclones.c (cgraph_node::create_virtual_clone):

> 	Create either IPA_REF_LOAD of IPA_REF_READ depending on

> 	whether new_tree is a VAR_DECL or an ADDR_EXPR.

> 	* ipa-cp.c (create_specialized_node): Add reference just for

> 	ADDR_EXPRs.

> 	* symtab.c (symtab_node::maybe_create_reference): Remove guard

> 	as it's guarded in callers.

> ---

>  gcc/cgraphclones.c | 6 +++++-

>  gcc/ipa-cp.c       | 3 ++-

>  gcc/symtab.c       | 2 --

>  3 files changed, 7 insertions(+), 4 deletions(-)

> 

> diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c

> index 349892dab67..6c8fe156f23 100644

> --- a/gcc/cgraphclones.c

> +++ b/gcc/cgraphclones.c

> @@ -624,7 +624,11 @@ cgraph_node::create_virtual_clone (vec<cgraph_edge *> redirect_callers,

>        || in_lto_p)

>      new_node->unique_name = true;

>    FOR_EACH_VEC_SAFE_ELT (tree_map, i, map)

> -    new_node->maybe_create_reference (map->new_tree, IPA_REF_ADDR, NULL);

> +    {

> +      ipa_ref_use use_type

> +	= TREE_CODE (map->new_tree) == ADDR_EXPR ? IPA_REF_ADDR : IPA_REF_LOAD;

> +      new_node->maybe_create_reference (map->new_tree, use_type, NULL);

> +    }

>  

>    if (ipa_transforms_to_apply.exists ())

>      new_node->ipa_transforms_to_apply

> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c

> index d3b50524457..fd312b56fde 100644

> --- a/gcc/ipa-cp.c

> +++ b/gcc/ipa-cp.c

> @@ -3787,7 +3787,8 @@ create_specialized_node (struct cgraph_node *node,

>  					 args_to_skip, "constprop");

>    ipa_set_node_agg_value_chain (new_node, aggvals);

>    for (av = aggvals; av; av = av->next)

> -    new_node->maybe_create_reference (av->value, IPA_REF_ADDR, NULL);

> +    if (TREE_CODE (av->value) == ADDR_EXPR)

> +      new_node->maybe_create_reference (av->value, IPA_REF_ADDR, NULL);

>  

>    if (dump_file && (dump_flags & TDF_DETAILS))

>      {

> diff --git a/gcc/symtab.c b/gcc/symtab.c

> index 73168a8db09..562a4a2f6a6 100644

> --- a/gcc/symtab.c

> +++ b/gcc/symtab.c

> @@ -598,8 +598,6 @@ symtab_node::maybe_create_reference (tree val, enum ipa_ref_use use_type,

>  				     gimple *stmt)

>  {

>    STRIP_NOPS (val);

> -  if (TREE_CODE (val) != ADDR_EXPR)

> -    return NULL;


Perhaps maybe_create_reference should drop the use_type argument (it is used
with IPA_REF_ADDR only anyway) and should do the parsing itself?
I.e. if there is reference do IPA_REF_LOAD and if there is ADDR_EXPR do
IPA_REF_ADDR.  Why one can not have handled component refs in there?

Honza
>    val = get_base_var (val);

>    if (val && VAR_OR_FUNCTION_DECL_P (val))

>      {

> -- 

> 2.11.0

>
Martin Liška Jan. 19, 2017, 3:02 p.m. UTC | #3 
On 01/18/2017 11:18 PM, Jan Hubicka wrote:
>>

>> 2016-12-19  Martin Liska  <mliska@suse.cz>

>>

>> 	* cgraphclones.c (cgraph_node::create_virtual_clone):

>> 	Create either IPA_REF_LOAD of IPA_REF_READ depending on

>> 	whether new_tree is a VAR_DECL or an ADDR_EXPR.

>> 	* ipa-cp.c (create_specialized_node): Add reference just for

>> 	ADDR_EXPRs.

>> 	* symtab.c (symtab_node::maybe_create_reference): Remove guard

>> 	as it's guarded in callers.

>> ---

>>  gcc/cgraphclones.c | 6 +++++-

>>  gcc/ipa-cp.c       | 3 ++-

>>  gcc/symtab.c       | 2 --

>>  3 files changed, 7 insertions(+), 4 deletions(-)

>>

>> diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c

>> index 349892dab67..6c8fe156f23 100644

>> --- a/gcc/cgraphclones.c

>> +++ b/gcc/cgraphclones.c

>> @@ -624,7 +624,11 @@ cgraph_node::create_virtual_clone (vec<cgraph_edge *> redirect_callers,

>>        || in_lto_p)

>>      new_node->unique_name = true;

>>    FOR_EACH_VEC_SAFE_ELT (tree_map, i, map)

>> -    new_node->maybe_create_reference (map->new_tree, IPA_REF_ADDR, NULL);

>> +    {

>> +      ipa_ref_use use_type

>> +	= TREE_CODE (map->new_tree) == ADDR_EXPR ? IPA_REF_ADDR : IPA_REF_LOAD;

>> +      new_node->maybe_create_reference (map->new_tree, use_type, NULL);

>> +    }

>>  

>>    if (ipa_transforms_to_apply.exists ())

>>      new_node->ipa_transforms_to_apply

>> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c

>> index d3b50524457..fd312b56fde 100644

>> --- a/gcc/ipa-cp.c

>> +++ b/gcc/ipa-cp.c

>> @@ -3787,7 +3787,8 @@ create_specialized_node (struct cgraph_node *node,

>>  					 args_to_skip, "constprop");

>>    ipa_set_node_agg_value_chain (new_node, aggvals);

>>    for (av = aggvals; av; av = av->next)

>> -    new_node->maybe_create_reference (av->value, IPA_REF_ADDR, NULL);

>> +    if (TREE_CODE (av->value) == ADDR_EXPR)

>> +      new_node->maybe_create_reference (av->value, IPA_REF_ADDR, NULL);

>>  

>>    if (dump_file && (dump_flags & TDF_DETAILS))

>>      {

>> diff --git a/gcc/symtab.c b/gcc/symtab.c

>> index 73168a8db09..562a4a2f6a6 100644

>> --- a/gcc/symtab.c

>> +++ b/gcc/symtab.c

>> @@ -598,8 +598,6 @@ symtab_node::maybe_create_reference (tree val, enum ipa_ref_use use_type,

>>  				     gimple *stmt)

>>  {

>>    STRIP_NOPS (val);

>> -  if (TREE_CODE (val) != ADDR_EXPR)

>> -    return NULL;

> 

> Perhaps maybe_create_reference should drop the use_type argument (it is used

> with IPA_REF_ADDR only anyway) and should do the parsing itself?

> I.e. if there is reference do IPA_REF_LOAD and if there is ADDR_EXPR do

> IPA_REF_ADDR.  Why one can not have handled component refs in there?

> 

> Honza


Ok, this is updated version that I've been testing. Added a reference to PR that
we just identified that is also caused by IPA CP and will be fixed by the patch.

Thanks,
Martin

>>    val = get_base_var (val);

>>    if (val && VAR_OR_FUNCTION_DECL_P (val))

>>      {

>> -- 

>> 2.11.0

>>

>From f42c5ea2ce09ecbf02e472ce31add53189115d66 Mon Sep 17 00:00:00 2001

From: marxin <mliska@suse.cz>

Date: Mon, 19 Dec 2016 11:03:34 +0100
Subject: [PATCH] Fix IPA CP where it forgot to add a reference in cgraph (PR
 ipa/71190).

gcc/ChangeLog:

2017-01-19  Martin Liska  <mliska@suse.cz>

	PR ipa/71190
	* cgraph.h (maybe_create_reference): Remove argument and
	update comment.
	* cgraphclones.c (cgraph_node::create_virtual_clone): Remove one
	argument.
	* ipa-cp.c (create_specialized_node): Likewise.
	* symtab.c (symtab_node::maybe_create_reference): Handle
	VAR_DECLs and ADDR_EXPRs and select ipa_ref_use type.
---
 gcc/cgraph.h       |  6 ++----
 gcc/cgraphclones.c |  2 +-
 gcc/ipa-cp.c       |  2 +-
 gcc/symtab.c       | 24 +++++++++++++++---------
 4 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index db2915c5751..5410a71176a 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -131,11 +131,9 @@ public:
 			     enum ipa_ref_use use_type, gimple *stmt);
 
   /* If VAL is a reference to a function or a variable, add a reference from
-     this symtab_node to the corresponding symbol table node.  USE_TYPE specify
-     type of the use and STMT the statement (if it exists).  Return the new
+     this symtab_node to the corresponding symbol table node.  Return the new
      reference or NULL if none was created.  */
-  ipa_ref *maybe_create_reference (tree val, enum ipa_ref_use use_type,
-				   gimple *stmt);
+  ipa_ref *maybe_create_reference (tree val, gimple *stmt);
 
   /* Clone all references from symtab NODE to this symtab_node.  */
   void clone_references (symtab_node *node);
diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index a17663519a9..c2337e84553 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -624,7 +624,7 @@ cgraph_node::create_virtual_clone (vec<cgraph_edge *> redirect_callers,
       || in_lto_p)
     new_node->unique_name = true;
   FOR_EACH_VEC_SAFE_ELT (tree_map, i, map)
-    new_node->maybe_create_reference (map->new_tree, IPA_REF_ADDR, NULL);
+    new_node->maybe_create_reference (map->new_tree, NULL);
 
   if (ipa_transforms_to_apply.exists ())
     new_node->ipa_transforms_to_apply
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 9cc903769e8..aa3c9973a66 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -3786,7 +3786,7 @@ create_specialized_node (struct cgraph_node *node,
 					 args_to_skip, "constprop");
   ipa_set_node_agg_value_chain (new_node, aggvals);
   for (av = aggvals; av; av = av->next)
-    new_node->maybe_create_reference (av->value, IPA_REF_ADDR, NULL);
+    new_node->maybe_create_reference (av->value, NULL);
 
   if (dump_file && (dump_flags & TDF_DETAILS))
     {
diff --git a/gcc/symtab.c b/gcc/symtab.c
index 87120970d34..64abf32ae13 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -588,18 +588,24 @@ symtab_node::create_reference (symtab_node *referred_node,
   return ref;
 }
 
-/* If VAL is a reference to a function or a variable, add a reference from
-   this symtab_node to the corresponding symbol table node.  USE_TYPE specify
-   type of the use and STMT the statement (if it exists).  Return the new
-   reference or NULL if none was created.  */
-
 ipa_ref *
-symtab_node::maybe_create_reference (tree val, enum ipa_ref_use use_type,
-				     gimple *stmt)
+symtab_node::maybe_create_reference (tree val, gimple *stmt)
 {
   STRIP_NOPS (val);
-  if (TREE_CODE (val) != ADDR_EXPR)
-    return NULL;
+  ipa_ref_use use_type;
+
+  switch (TREE_CODE (val))
+    {
+    case VAR_DECL:
+      use_type = IPA_REF_LOAD;
+      break;
+    case ADDR_EXPR:
+      use_type = IPA_REF_ADDR;
+      break;
+    default:
+      return NULL;
+    }
+
   val = get_base_var (val);
   if (val && VAR_OR_FUNCTION_DECL_P (val))
     {
-- 
2.11.0
Jan Hubicka Jan. 19, 2017, 3:10 p.m. UTC | #4 
> >> 2016-12-19  Martin Liska  <mliska@suse.cz>

> >>

> >> 	* cgraphclones.c (cgraph_node::create_virtual_clone):

> >> 	Create either IPA_REF_LOAD of IPA_REF_READ depending on

> >> 	whether new_tree is a VAR_DECL or an ADDR_EXPR.

> >> 	* ipa-cp.c (create_specialized_node): Add reference just for

> >> 	ADDR_EXPRs.

> >> 	* symtab.c (symtab_node::maybe_create_reference): Remove guard

> >> 	as it's guarded in callers.


Path is OK
>  ipa_ref *

> -symtab_node::maybe_create_reference (tree val, enum ipa_ref_use use_type,

> -				     gimple *stmt)

> +symtab_node::maybe_create_reference (tree val, gimple *stmt)

>  {

>    STRIP_NOPS (val);

> -  if (TREE_CODE (val) != ADDR_EXPR)

> -    return NULL;

> +  ipa_ref_use use_type;

> +

> +  switch (TREE_CODE (val))

> +    {

> +    case VAR_DECL:

> +      use_type = IPA_REF_LOAD;

> +      break;

> +    case ADDR_EXPR:

> +      use_type = IPA_REF_ADDR;

> +      break;

> +    default:

> +      return NULL;

> +    }


I would add assert into default that we don't get handled_component_ref here so we are sure
we don't miss any declarations (because the bug leads to quite esoteric issues, it is better
to be safe than sorry)

Honza
Martin Liška Jan. 20, 2017, 8:44 a.m. UTC | #5 
On 01/19/2017 04:10 PM, Jan Hubicka wrote:
>>>> 2016-12-19  Martin Liska  <mliska@suse.cz>

>>>>

>>>> 	* cgraphclones.c (cgraph_node::create_virtual_clone):

>>>> 	Create either IPA_REF_LOAD of IPA_REF_READ depending on

>>>> 	whether new_tree is a VAR_DECL or an ADDR_EXPR.

>>>> 	* ipa-cp.c (create_specialized_node): Add reference just for

>>>> 	ADDR_EXPRs.

>>>> 	* symtab.c (symtab_node::maybe_create_reference): Remove guard

>>>> 	as it's guarded in callers.

> 

> Path is OK

>>  ipa_ref *

>> -symtab_node::maybe_create_reference (tree val, enum ipa_ref_use use_type,

>> -				     gimple *stmt)

>> +symtab_node::maybe_create_reference (tree val, gimple *stmt)

>>  {

>>    STRIP_NOPS (val);

>> -  if (TREE_CODE (val) != ADDR_EXPR)

>> -    return NULL;

>> +  ipa_ref_use use_type;

>> +

>> +  switch (TREE_CODE (val))

>> +    {

>> +    case VAR_DECL:

>> +      use_type = IPA_REF_LOAD;

>> +      break;

>> +    case ADDR_EXPR:

>> +      use_type = IPA_REF_ADDR;

>> +      break;

>> +    default:

>> +      return NULL;

>> +    }

> 

> I would add assert into default that we don't get handled_component_ref here so we are sure

> we don't miss any declarations (because the bug leads to quite esoteric issues, it is better

> to be safe than sorry)

> 

> Honza

> 


Done in patch I've just installed as r244687.

Thanks for help,
MartinFrom b0c002ba76312ba7cf38a41b1127ae8a55e89639 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>

Date: Mon, 19 Dec 2016 11:03:34 +0100
Subject: [PATCH] Fix IPA CP where it forgot to add a reference in cgraph (PR
 ipa/71190).

gcc/ChangeLog:

2017-01-19  Martin Liska  <mliska@suse.cz>

	PR ipa/71190
	* cgraph.h (maybe_create_reference): Remove argument and
	update comment.
	* cgraphclones.c (cgraph_node::create_virtual_clone): Remove one
	argument.
	* ipa-cp.c (create_specialized_node): Likewise.
	* symtab.c (symtab_node::maybe_create_reference): Handle
	VAR_DECLs and ADDR_EXPRs and select ipa_ref_use type.
---
 gcc/cgraph.h       |  6 ++----
 gcc/cgraphclones.c |  2 +-
 gcc/ipa-cp.c       |  2 +-
 gcc/symtab.c       | 25 ++++++++++++++++---------
 4 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index db2915c5751..5410a71176a 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -131,11 +131,9 @@ public:
 			     enum ipa_ref_use use_type, gimple *stmt);
 
   /* If VAL is a reference to a function or a variable, add a reference from
-     this symtab_node to the corresponding symbol table node.  USE_TYPE specify
-     type of the use and STMT the statement (if it exists).  Return the new
+     this symtab_node to the corresponding symbol table node.  Return the new
      reference or NULL if none was created.  */
-  ipa_ref *maybe_create_reference (tree val, enum ipa_ref_use use_type,
-				   gimple *stmt);
+  ipa_ref *maybe_create_reference (tree val, gimple *stmt);
 
   /* Clone all references from symtab NODE to this symtab_node.  */
   void clone_references (symtab_node *node);
diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index a17663519a9..c2337e84553 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -624,7 +624,7 @@ cgraph_node::create_virtual_clone (vec<cgraph_edge *> redirect_callers,
       || in_lto_p)
     new_node->unique_name = true;
   FOR_EACH_VEC_SAFE_ELT (tree_map, i, map)
-    new_node->maybe_create_reference (map->new_tree, IPA_REF_ADDR, NULL);
+    new_node->maybe_create_reference (map->new_tree, NULL);
 
   if (ipa_transforms_to_apply.exists ())
     new_node->ipa_transforms_to_apply
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 9cc903769e8..aa3c9973a66 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -3786,7 +3786,7 @@ create_specialized_node (struct cgraph_node *node,
 					 args_to_skip, "constprop");
   ipa_set_node_agg_value_chain (new_node, aggvals);
   for (av = aggvals; av; av = av->next)
-    new_node->maybe_create_reference (av->value, IPA_REF_ADDR, NULL);
+    new_node->maybe_create_reference (av->value, NULL);
 
   if (dump_file && (dump_flags & TDF_DETAILS))
     {
diff --git a/gcc/symtab.c b/gcc/symtab.c
index 87120970d34..c21e7acf28c 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -588,18 +588,25 @@ symtab_node::create_reference (symtab_node *referred_node,
   return ref;
 }
 
-/* If VAL is a reference to a function or a variable, add a reference from
-   this symtab_node to the corresponding symbol table node.  USE_TYPE specify
-   type of the use and STMT the statement (if it exists).  Return the new
-   reference or NULL if none was created.  */
-
 ipa_ref *
-symtab_node::maybe_create_reference (tree val, enum ipa_ref_use use_type,
-				     gimple *stmt)
+symtab_node::maybe_create_reference (tree val, gimple *stmt)
 {
   STRIP_NOPS (val);
-  if (TREE_CODE (val) != ADDR_EXPR)
-    return NULL;
+  ipa_ref_use use_type;
+
+  switch (TREE_CODE (val))
+    {
+    case VAR_DECL:
+      use_type = IPA_REF_LOAD;
+      break;
+    case ADDR_EXPR:
+      use_type = IPA_REF_ADDR;
+      break;
+    default:
+      gcc_assert (!handled_component_p (val));
+      return NULL;
+    }
+
   val = get_base_var (val);
   if (val && VAR_OR_FUNCTION_DECL_P (val))
     {
-- 
2.11.0
diff mbox

Patch

From 2e29080e44cce899f9d5181185aba0a8a8791a9a Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 19 Dec 2016 11:03:34 +0100
Subject: [PATCH] Fix IPA CP where it forgot to add a reference in cgraph

gcc/ChangeLog:

2016-12-19  Martin Liska  <mliska@suse.cz>

	* cgraphclones.c (cgraph_node::create_virtual_clone):
	Create either IPA_REF_LOAD of IPA_REF_READ depending on
	whether new_tree is a VAR_DECL or an ADDR_EXPR.
	* ipa-cp.c (create_specialized_node): Add reference just for
	ADDR_EXPRs.
	* symtab.c (symtab_node::maybe_create_reference): Remove guard
	as it's guarded in callers.
---
 gcc/cgraphclones.c | 6 +++++-
 gcc/ipa-cp.c       | 3 ++-
 gcc/symtab.c       | 2 --
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index 349892dab67..6c8fe156f23 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -624,7 +624,11 @@  cgraph_node::create_virtual_clone (vec<cgraph_edge *> redirect_callers,
       || in_lto_p)
     new_node->unique_name = true;
   FOR_EACH_VEC_SAFE_ELT (tree_map, i, map)
-    new_node->maybe_create_reference (map->new_tree, IPA_REF_ADDR, NULL);
+    {
+      ipa_ref_use use_type
+	= TREE_CODE (map->new_tree) == ADDR_EXPR ? IPA_REF_ADDR : IPA_REF_LOAD;
+      new_node->maybe_create_reference (map->new_tree, use_type, NULL);
+    }
 
   if (ipa_transforms_to_apply.exists ())
     new_node->ipa_transforms_to_apply
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index d3b50524457..fd312b56fde 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -3787,7 +3787,8 @@  create_specialized_node (struct cgraph_node *node,
 					 args_to_skip, "constprop");
   ipa_set_node_agg_value_chain (new_node, aggvals);
   for (av = aggvals; av; av = av->next)
-    new_node->maybe_create_reference (av->value, IPA_REF_ADDR, NULL);
+    if (TREE_CODE (av->value) == ADDR_EXPR)
+      new_node->maybe_create_reference (av->value, IPA_REF_ADDR, NULL);
 
   if (dump_file && (dump_flags & TDF_DETAILS))
     {
diff --git a/gcc/symtab.c b/gcc/symtab.c
index 73168a8db09..562a4a2f6a6 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -598,8 +598,6 @@  symtab_node::maybe_create_reference (tree val, enum ipa_ref_use use_type,
 				     gimple *stmt)
 {
   STRIP_NOPS (val);
-  if (TREE_CODE (val) != ADDR_EXPR)
-    return NULL;
   val = get_base_var (val);
   if (val && VAR_OR_FUNCTION_DECL_P (val))
     {
-- 
2.11.0