[PATCH] ICF: move readonly decision for variables to the right place

[PATCH] ICF: move readonly decision for variables to the right place

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 188 bytes --]

Hello.

This patch makes correctly decision about variables that are read-only and are candidates
for merge operation. Tested on x86_64-unknown-linux-gnu.

Ready for trunk?
Thanks,
Martin

[-- Attachment #2: 0003-ICF-Optimize-variable-merging.patch --]
[-- Type: text/x-patch, Size: 2670 bytes --]

From 72370368eb04a69bf36b42f692d44c21592bc94a Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Fri, 27 Feb 2015 23:44:32 +0100
Subject: [PATCH 3/4] ICF: Optimize variable merging.

gcc/ChangeLog:

2015-02-28  Martin Liska  <mliska@suse.cz>
	    Jan Hubicka   <hubicka@ucw.cz>

	* ipa-icf.c (sem_variable::parse): Do not filter variables
	too early.
	(sem_item_optimizer::filter_removed_items): Filter out correct
	readonly variables.

gcc/testsuite/ChangeLog:

2015-02-28  Martin Liska  <mliska@suse.cz>
	    Jan Hubicka   <hubicka@ucw.cz>

	* gcc.dg/ipa/ipa-icf-35.c: New test.
---
 gcc/ipa-icf.c                         | 13 ++++++++-----
 gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index 864a5d0..5b1fcff 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1410,10 +1410,6 @@ sem_variable::parse (varpool_node *node, bitmap_obstack *stack)
   if (node->alias)
     return NULL;
 
-  bool readonly = TYPE_P (decl) ? TYPE_READONLY (decl) : TREE_READONLY (decl);
-  if (!readonly)
-    return NULL;
-
   bool can_handle = DECL_VIRTUAL_P (decl)
 		    || flag_merge_constants >= 2
 		    || (!TREE_ADDRESSABLE (decl) && !node->externally_visible);
@@ -1900,7 +1896,14 @@ sem_item_optimizer::filter_removed_items (void)
 	  if (!flag_ipa_icf_variables)
 	    remove_item (item);
 	  else
-	    filtered.safe_push (item);
+	    {
+	      /* Filter out non-readonly variables.  */
+	      tree decl = item->decl;
+	      if (TYPE_P (decl) ? TYPE_READONLY (decl) : TREE_READONLY (decl))
+		filtered.safe_push (item);
+	      else
+		remove_item (item);
+	    }
         }
     }
 
diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c
new file mode 100644
index 0000000..95d247e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-35.c
@@ -0,0 +1,34 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-icf"  } */
+
+#include <stdlib.h>
+#include <assert.h>
+
+void f1()
+{
+}
+
+void f2()
+{
+}
+
+static void (*a)(void)=&f1;
+static void (*b)(void)=&f1;
+static void (*c)(void)=&f2;
+static void (*d)(void)=&f2;
+
+int main()
+{
+  a();
+  b();
+  c();
+  d();
+
+  return 0;
+}
+
+/* { dg-final { scan-ipa-dump "Equal symbols: 3" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:f2->f1" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:d->c" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:b->a" "icf"  } } */
+/* { dg-final { cleanup-ipa-dump "icf" } } */
-- 
2.1.2

Re: [PATCH] ICF: move readonly decision for variables to the right place

[Jan Hubicka]

> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
> index 864a5d0..5b1fcff 100644
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -1410,10 +1410,6 @@ sem_variable::parse (varpool_node *node, bitmap_obstack *stack)
>    if (node->alias)
>      return NULL;
>  
> -  bool readonly = TYPE_P (decl) ? TYPE_READONLY (decl) : TREE_READONLY (decl);
> -  if (!readonly)
> -    return NULL;
> -
>    bool can_handle = DECL_VIRTUAL_P (decl)
>  		    || flag_merge_constants >= 2
>  		    || (!TREE_ADDRESSABLE (decl) && !node->externally_visible);

Please also drop can_handle and DECL_EXTERNAL check here.

can_handle seems to test if address matters. We already verify it at merging time
and at a time we consider REF_ADDR references. This is enough.
Moreover TREE_ADDRESSABLE and externally_visible flags are subject to change in between
analysis and WPA time.

Instead of this punt if THIS_VOLATILE is true.

Instead of ctor_for_folding bellow use DECL_INITIAL (ctor_for_folding may return
you NULL if it thinks the var is not safe)

> @@ -1900,7 +1896,14 @@ sem_item_optimizer::filter_removed_items (void)
>  	  if (!flag_ipa_icf_variables)
>  	    remove_item (item);
>  	  else
> -	    filtered.safe_push (item);
> +	    {
> +	      /* Filter out non-readonly variables.  */
> +	      tree decl = item->decl;
> +	      if (TYPE_P (decl) ? TYPE_READONLY (decl) : TREE_READONLY (decl))

Just test TREE_READONLY (decl).

OK with these changes.
Honza

Re: [PATCH] ICF: move readonly decision for variables to the right place

[Jan Hubicka]

Hi,
my comments to your patch was not complete. ipa-icf is quite confused about
how to get the constructor.  This is how it works:

In during in_lto_p DECL_INITIAL may be set to error_mark_node and once you
really want to have the constructor there is varpool_node->get_constuctor
method. Because in majority cases we care about constructor either when we
output the initializer or when we want to use it for folding I also added
function ctor_for_fodling that returns the constructor or error_mark_node
when for some reason the folding is unsafe (for example the variable is
interposable).

Now ipa-ICF actually calls ctor_for_folding for every variable it considers
that will lead to a lot of streaming at WPA time, but it also check
DECL_INITIAL to not be error_mark_node, so it think at WPA, the unification was
limited to virtual tables only in case the ctors was already brought in by the
devirtualization machinery.

This patch prepares for the actual fix by introducing WPA comparsion the
same was as already done for function.

I also proofread sem_variable::equals and compared it to varasm.c
compare_constant that is doing exactly the same to unify constant pool
variables. There were quite few disprepancies (such as forgotten checks for
start of arrays), so I decided to bring the implementationfrom varasm.c as it
is better tested. The code is bit fancier trying to prove equivalence despite
bit different representation, but not by too much.

There are two additional fixes.  I forbidden mixing data from different
sections.  In the cases like kernel, where one section may be discarded
from memory this is clearly undesirable.  I also noticed that we fixed the
CONSTANT_POOL problem twice, so removed the check in sem_variable::equals.

For complete paranoia I also forbidden unification of DECL_VIRTUAL and
!DECL_VIRTUAL.  One cares about addresses while other doesn't.  I do not think
it can make difference in practice as there is no valid way to write identical
datastructure to a virtual table.

Next stage1 we ought to unify the code and perhaps use ICF macinery
to replace tree constant pool.

Bootstrapped/regtested x86_64-linux, comited.

Honza

	* ipa-icf.c: Include stor-layout.h
	(sem_function::compare_cgraph_references): Rename to ...
	(sem_item::compare_cgraph_references): ... this one.
	(sem_variable::equals_wpa): New function
	(sem_variable::equals): Do not check stuff already verified by
	equals_wpa.
	(sem_variable::equals): Reorg based on varasm.c:compare_constant.
	* ipa-icf.h (sem_item): Add compare_cgraph_references.
	(sem_function): Remove compare_cgraph_references.
	(sem_variable): Turns equals_wpa into non-inline.
Index: ipa-icf.c
===================================================================
--- ipa-icf.c	(revision 221092)
+++ ipa-icf.c	(working copy)
@@ -53,6 +53,7 @@ along with GCC; see the file COPYING3.
 
 #include "config.h"
 #include "system.h"
+#include <list>
 #include "coretypes.h"
 #include "hash-set.h"
 #include "machmode.h"
@@ -119,9 +120,9 @@ along with GCC; see the file COPYING3.
 #include "lto-streamer.h"
 #include "data-streamer.h"
 #include "ipa-utils.h"
-#include <list>
 #include "ipa-icf-gimple.h"
 #include "ipa-icf.h"
+#include "stor-layout.h"
 
 using namespace ipa_icf_gimple;
 
@@ -338,7 +339,7 @@ sem_function::get_hash (void)
    contains these nodes.  ADDRESS indicate if address is taken.  */
 
 bool
-sem_function::compare_cgraph_references (
+sem_item::compare_cgraph_references (
     hash_map <symtab_node *, sem_item *> &ignored_nodes,
     symtab_node *n1, symtab_node *n2, bool address)
 {
@@ -1395,6 +1396,58 @@ sem_variable::sem_variable (varpool_node
   gcc_checking_assert (get_node ());
 }
 
+/* Fast equality function based on knowledge known in WPA.  */
+
+bool
+sem_variable::equals_wpa (sem_item *item,
+			  hash_map <symtab_node *, sem_item *> &ignored_nodes)
+{
+  gcc_assert (item->type == VAR);
+
+  sem_variable *compared_var = static_cast<sem_variable *> (item);
+
+  if (node->num_references () != item->node->num_references ())
+    return return_false_with_msg ("different number of references");
+
+  if (DECL_TLS_MODEL (decl) || DECL_TLS_MODEL (item->decl))
+    return return_false_with_msg ("TLS model");
+
+  if (DECL_ALIGN (decl) != DECL_ALIGN (item->decl))
+    return return_false_with_msg ("alignment mismatch");
+
+  if (DECL_VIRTUAL_P (decl) != DECL_VIRTUAL_P (item->decl))
+    return return_false_with_msg ("Virtual flag mismatch");
+
+  if (DECL_SIZE (decl) != DECL_SIZE (item->decl)
+      && ((!DECL_SIZE (decl) || !DECL_SIZE (item->decl))
+	  || !operand_equal_p (DECL_SIZE (decl),
+			       DECL_SIZE (item->decl), OEP_ONLY_CONST)))
+    return return_false_with_msg ("size mismatch");
+
+  /* Do not attempt to mix data from different user sections;
+     we do not know what user intends with those.  */
+  if (((DECL_SECTION_NAME (decl) && !node->implicit_section)
+       || (DECL_SECTION_NAME (item->decl) && !item->node->implicit_section))
+      && DECL_SECTION_NAME (decl) != DECL_SECTION_NAME (item->decl))
+    return return_false_with_msg ("user section mismatch");
+
+  if (DECL_IN_TEXT_SECTION (decl) != DECL_IN_TEXT_SECTION (item->decl))
+    return return_false_with_msg ("text section");
+
+  ipa_ref *ref = NULL, *ref2 = NULL;
+  for (unsigned i = 0; node->iterate_reference (i, ref); i++)
+    {
+      item->node->iterate_reference (i, ref2);
+
+      if (!compare_cgraph_references (ignored_nodes,
+				      ref->referred, ref2->referred,
+				      ref->address_matters_p ()))
+	return false;
+    }
+
+  return true;
+}
+
 /* Returns true if the item equals to ITEM given as argument.  */
 
 bool
@@ -1408,17 +1461,6 @@ sem_variable::equals (sem_item *item,
   if (!ctor || !v->ctor)
     return return_false_with_msg ("ctor is missing for semantic variable");
 
-  if (DECL_IN_CONSTANT_POOL (decl)
-      && (DECL_IN_CONSTANT_POOL (item->decl)
-	  || item->node->address_matters_p ()))
-    return return_false_with_msg ("constant pool");
-
-  if (DECL_IN_TEXT_SECTION (decl) != DECL_IN_TEXT_SECTION (item->decl))
-    return return_false_with_msg ("text section");
-
-  if (DECL_TLS_MODEL (decl) || DECL_TLS_MODEL (item->decl))
-    return return_false_with_msg ("TLS model");
-
   return sem_variable::equals (ctor, v->ctor);
 }
 
@@ -1427,28 +1469,57 @@ sem_variable::equals (sem_item *item,
 bool
 sem_variable::equals (tree t1, tree t2)
 {
+  if (!t1 || !t2)
+    return return_with_debug (t1 == t2);
+  if (t1 == t2)
+    return true;
   tree_code tc1 = TREE_CODE (t1);
   tree_code tc2 = TREE_CODE (t2);
 
   if (tc1 != tc2)
-    return false;
+    return return_false_with_msg ("TREE_CODE mismatch");
 
   switch (tc1)
     {
     case CONSTRUCTOR:
       {
-	unsigned len1 = vec_safe_length (CONSTRUCTOR_ELTS (t1));
-	unsigned len2 = vec_safe_length (CONSTRUCTOR_ELTS (t2));
+	vec<constructor_elt, va_gc> *v1, *v2;
+	unsigned HOST_WIDE_INT idx;
 
-	if (len1 != len2)
-	  return false;
+	enum tree_code typecode = TREE_CODE (TREE_TYPE (t1));
+	if (typecode != TREE_CODE (TREE_TYPE (t2)))
+	  return return_false_with_msg ("constructor type mismatch");
 
-	for (unsigned i = 0; i < len1; i++)
-	  if (!sem_variable::equals (CONSTRUCTOR_ELT (t1, i)->value,
-				     CONSTRUCTOR_ELT (t2, i)->value)
-	      || CONSTRUCTOR_ELT (t1, i)->index != CONSTRUCTOR_ELT (t2, i)->index)
-	    return false;
+	if (typecode == ARRAY_TYPE)
+	  {
+	    HOST_WIDE_INT size_1 = int_size_in_bytes (TREE_TYPE (t1));
+	    /* For arrays, check that the sizes all match.  */
+	    if (TYPE_MODE (TREE_TYPE (t1)) != TYPE_MODE (TREE_TYPE (t2))
+		|| size_1 == -1
+		|| size_1 != int_size_in_bytes (TREE_TYPE (t2)))
+	      return return_false_with_msg ("constructor array size mismatch");
+	  }
+	else if (!func_checker::compatible_types_p (TREE_TYPE (t1),
+						    TREE_TYPE (t2)))
+	  return return_false_with_msg ("constructor type incompatible");
+
+	v1 = CONSTRUCTOR_ELTS (t1);
+	v2 = CONSTRUCTOR_ELTS (t2);
+	if (vec_safe_length (v1) != vec_safe_length (v2))
+	  return return_false_with_msg ("constructor number of elts mismatch");
 
+	for (idx = 0; idx < vec_safe_length (v1); ++idx)
+	  {
+	    constructor_elt *c1 = &(*v1)[idx];
+	    constructor_elt *c2 = &(*v2)[idx];
+
+	    /* Check that each value is the same...  */
+	    if (!sem_variable::equals (c1->value, c2->value))
+	      return false;
+	    /* ... and that they apply to the same fields!  */
+	    if (!sem_variable::equals (c1->index, c2->index))
+	      return false;
+	  }
 	return true;
       }
     case MEM_REF:
@@ -1463,32 +1534,100 @@ sem_variable::equals (tree t1, tree t2)
 	  return return_false ();
 
 	/* Type of the offset on MEM_REF does not matter.  */
-	return sem_variable::equals (x1, x2)
-	       && wi::to_offset  (y1) == wi::to_offset  (y2);
+	return return_with_debug (sem_variable::equals (x1, x2)
+			          && wi::to_offset  (y1)
+				     == wi::to_offset  (y2));
       }
-    case NOP_EXPR:
     case ADDR_EXPR:
+    case FDESC_EXPR:
       {
 	tree op1 = TREE_OPERAND (t1, 0);
 	tree op2 = TREE_OPERAND (t2, 0);
 	return sem_variable::equals (op1, op2);
       }
+    /* References to other vars/decls are compared using ipa-ref.  */
     case FUNCTION_DECL:
     case VAR_DECL:
+      if (decl_in_symtab_p (t1) && decl_in_symtab_p (t2))
+	return true;
+      return return_false_with_msg ("Declaration mismatch");
+    case CONST_DECL:
+      /* TODO: We can check CONST_DECL by its DECL_INITIAL, but for that we
+	 need to process its VAR/FUNCTION references without relying on ipa-ref
+	 compare.  */
     case FIELD_DECL:
     case LABEL_DECL:
-      return t1 == t2;
+      return return_false_with_msg ("Declaration mismatch");
     case INTEGER_CST:
-      return func_checker::compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2),
-	     true)
-	     && wi::to_offset (t1) == wi::to_offset (t2);
+      /* Integer constants are the same only if the same width of type.  */
+      if (TYPE_PRECISION (TREE_TYPE (t1)) != TYPE_PRECISION (TREE_TYPE (t2)))
+        return return_false_with_msg ("INTEGER_CST precision mismatch");
+      if (TYPE_MODE (TREE_TYPE (t1)) != TYPE_MODE (TREE_TYPE (t2)))
+        return return_false_with_msg ("INTEGER_CST mode mismatch");
+      return return_with_debug (tree_int_cst_equal (t1, t2));
     case STRING_CST:
-    case REAL_CST:
+      if (TYPE_MODE (TREE_TYPE (t1)) != TYPE_MODE (TREE_TYPE (t2)))
+        return return_false_with_msg ("STRING_CST mode mismatch");
+      if (TREE_STRING_LENGTH (t1) != TREE_STRING_LENGTH (t2))
+	return return_false_with_msg ("STRING_CST length mismatch");
+      if (memcmp (TREE_STRING_POINTER (t1), TREE_STRING_POINTER (t2),
+		    TREE_STRING_LENGTH (t1)))
+	return return_false_with_msg ("STRING_CST mismatch");
+      return true;
+    case FIXED_CST:
+      /* Fixed constants are the same only if the same width of type.  */
+      if (TYPE_PRECISION (TREE_TYPE (t1)) != TYPE_PRECISION (TREE_TYPE (t2)))
+        return return_false_with_msg ("FIXED_CST precision mismatch");
+
+      return return_with_debug (FIXED_VALUES_IDENTICAL (TREE_FIXED_CST (t1),
+							TREE_FIXED_CST (t2)));
     case COMPLEX_CST:
-      return operand_equal_p (t1, t2, OEP_ONLY_CONST);
-    case COMPONENT_REF:
+      return (sem_variable::equals (TREE_REALPART (t1), TREE_REALPART (t2))
+	      && sem_variable::equals (TREE_IMAGPART (t1), TREE_IMAGPART (t2)));
+    case REAL_CST:
+      /* Real constants are the same only if the same width of type.  */
+      if (TYPE_PRECISION (TREE_TYPE (t1)) != TYPE_PRECISION (TREE_TYPE (t2)))
+        return return_false_with_msg ("REAL_CST precision mismatch");
+      return return_with_debug (REAL_VALUES_IDENTICAL (TREE_REAL_CST (t1),
+						       TREE_REAL_CST (t2)));
+    case VECTOR_CST:
+      {
+	unsigned i;
+
+        if (VECTOR_CST_NELTS (t1) != VECTOR_CST_NELTS (t2))
+          return return_false_with_msg ("VECTOR_CST nelts mismatch");
+
+	for (i = 0; i < VECTOR_CST_NELTS (t1); ++i)
+	  if (!sem_variable::equals (VECTOR_CST_ELT (t1, i),
+				     VECTOR_CST_ELT (t2, i)))
+	    return 0;
+
+	return 1;
+      }
     case ARRAY_REF:
+    case ARRAY_RANGE_REF:
+      {
+	tree x1 = TREE_OPERAND (t1, 0);
+	tree x2 = TREE_OPERAND (t2, 0);
+	tree y1 = TREE_OPERAND (t1, 1);
+	tree y2 = TREE_OPERAND (t2, 1);
+
+	if (!sem_variable::equals (x1, x2) && sem_variable::equals (y1, y2))
+	  return false;
+	if (!sem_variable::equals (array_ref_low_bound (t1),
+				   array_ref_low_bound (t2)))
+	  return false;
+        if (!sem_variable::equals (array_ref_element_size (t1),
+			           array_ref_element_size (t2)))
+	  return false;
+	return true;
+      }
+     
+    case COMPONENT_REF:
     case POINTER_PLUS_EXPR:
+    case PLUS_EXPR:
+    case MINUS_EXPR:
+    case RANGE_EXPR:
       {
 	tree x1 = TREE_OPERAND (t1, 0);
 	tree x2 = TREE_OPERAND (t2, 0);
@@ -1497,6 +1636,13 @@ sem_variable::equals (tree t1, tree t2)
 
 	return sem_variable::equals (x1, x2) && sem_variable::equals (y1, y2);
       }
+
+    CASE_CONVERT:
+    case VIEW_CONVERT_EXPR:
+      if (!func_checker::compatible_types_p (TREE_TYPE (t1), TREE_TYPE (t2),
+					     true))
+	  return return_false ();
+      return sem_variable::equals (TREE_OPERAND (t1, 0), TREE_OPERAND (t2, 0));
     case ERROR_MARK:
       return return_false_with_msg ("ERROR_MARK");
     default:
Index: ipa-icf.h
===================================================================
--- ipa-icf.h	(revision 221092)
+++ ipa-icf.h	(working copy)
@@ -241,9 +241,18 @@ public:
 protected:
   /* Cached, once calculated hash for the item.  */
   hashval_t hash;
+
   /* Accumulate to HSTATE a hash of constructor expression EXP.  */
   static void add_expr (const_tree exp, inchash::hash &hstate);
 
+  /* For a given symbol table nodes N1 and N2, we check that FUNCTION_DECLs
+     point to a same function. Comparison can be skipped if IGNORED_NODES
+     contains these nodes.  ADDRESS indicate if address is taken.  */
+  bool compare_cgraph_references (hash_map <symtab_node *, sem_item *>
+				  &ignored_nodes,
+				  symtab_node *n1, symtab_node *n2,
+				  bool address);
+
 private:
   /* Initialize internal data structures. Bitmap STACK is used for
      bitmap memory allocation process.  */
@@ -353,14 +362,6 @@ private:
      ICF flags are the same.  */
   bool compare_edge_flags (cgraph_edge *e1, cgraph_edge *e2);
 
-  /* For a given symbol table nodes N1 and N2, we check that FUNCTION_DECLs
-     point to a same function. Comparison can be skipped if IGNORED_NODES
-     contains these nodes.  ADDRESS indicate if address is taken.  */
-  bool compare_cgraph_references (hash_map <symtab_node *, sem_item *>
-				  &ignored_nodes,
-				  symtab_node *n1, symtab_node *n2,
-				  bool address);
-
   /* Processes function equality comparison.  */
   bool equals_private (sem_item *item,
 		       hash_map <symtab_node *, sem_item *> &ignored_nodes);
@@ -402,12 +403,8 @@ public:
 		       hash_map <symtab_node *, sem_item *> &ignored_nodes);
 
   /* Fast equality variable based on knowledge known in WPA.  */
-  inline virtual bool equals_wpa (sem_item *item,
-				  hash_map <symtab_node *, sem_item *> & ARG_UNUSED(ignored_nodes))
-  {
-    gcc_assert (item->type == VAR);
-    return true;
-  }
+  virtual bool equals_wpa (sem_item *item,
+			   hash_map <symtab_node *, sem_item *> &ignored_nodes);
 
   /* Returns varpool_node.  */
   inline varpool_node *get_node (void)

Re: [PATCH] ICF: move readonly decision for variables to the right place

[Jan Hubicka]

Hi,
this is a variant of patch I commited.
It takes care to load the constructor to memory in sem_variable::equals
and donot touch it earlier.  I also made sem_variable::parse to skip volatile
and reigster variables.

Bootstrapped/regtested x86_64-linux, lto-bootstrapped with -fmerge-all-constants
comitted.

Honza

2015-02-28  Martin Liska  <mliska@suse.cz>
	    Jan Hubicka   <hubicka@ucw.cz>

	* ipa-icf.c (sem_variable::equals): Improve debug output;
	get variable constructor.
	(sem_variable::parse): Do not filter out too early; give up on
	volatile and register vars.
	(sem_item_optimizer::filter_removed_items): Filter out nonreadonly
	variables.
	* ipa-icf.h (sem_variable::init): Do not set ctor.
	(sem_variable::ctor): Remove.

gcc/testsuite/ChangeLog:

2015-02-28  Martin Liska  <mliska@suse.cz>
	    Jan Hubicka   <hubicka@ucw.cz>

	* gcc.dg/ipa/ipa-icf-35.c: New test.
	* gcc.dg/ipa/ipa-icf-36.c: New test.
	* gcc.dg/ipa/ipa-icf-37.c: New test.
Index: ipa-icf.c
===================================================================
--- ipa-icf.c	(revision 221096)
+++ ipa-icf.c	(working copy)
@@ -1448,18 +1452,29 @@ sem_variable::equals_wpa (sem_item *item
 
 /* Returns true if the item equals to ITEM given as argument.  */
 
+/* Returns true if the item equals to ITEM given as argument.  */
+
 bool
 sem_variable::equals (sem_item *item,
-		      hash_map <symtab_node *, sem_item *> & ARG_UNUSED (ignored_nodes))
+		      hash_map <symtab_node *, sem_item *> &)
 {
   gcc_assert (item->type == VAR);
+  bool ret;
 
-  sem_variable *v = static_cast<sem_variable *>(item);
-
-  if (!ctor || !v->ctor)
-    return return_false_with_msg ("ctor is missing for semantic variable");
+  if (DECL_INITIAL (decl) == error_mark_node && in_lto_p)
+    dyn_cast <varpool_node *>(node)->get_constructor ();
+  if (DECL_INITIAL (item->decl) == error_mark_node && in_lto_p)
+    dyn_cast <varpool_node *>(item->node)->get_constructor ();
+
+  ret = sem_variable::equals (DECL_INITIAL (decl),
+			      DECL_INITIAL (item->node->decl));
+  if (dump_file && (dump_flags & TDF_DETAILS))
+    fprintf (dump_file,
+	     "Equals called for vars:%s:%s (%u:%u) (%s:%s) with result: %s\n\n",
+	     name(), item->name (), node->order, item->node->order, asm_name (),
+	     item->asm_name (), ret ? "true" : "false");
 
-  return sem_variable::equals (ctor, v->ctor);
+  return ret;
 }
 
 /* Compares trees T1 and T2 for semantic equality.  */
@@ -1653,24 +1668,7 @@ sem_variable::equals (tree t1, tree t2)
 sem_variable *
 sem_variable::parse (varpool_node *node, bitmap_obstack *stack)
 {
-  tree decl = node->decl;
-
-  if (node->alias)
-    return NULL;
-
-  bool readonly = TYPE_P (decl) ? TYPE_READONLY (decl) : TREE_READONLY (decl);
-  if (!readonly)
-    return NULL;
-
-  bool can_handle = DECL_VIRTUAL_P (decl)
-		    || flag_merge_constants >= 2
-		    || (!TREE_ADDRESSABLE (decl) && !node->externally_visible);
-
-  if (!can_handle || DECL_EXTERNAL (decl))
-    return NULL;
-
-  tree ctor = ctor_for_folding (decl);
-  if (!ctor)
+  if (TREE_THIS_VOLATILE (node->decl) || DECL_HARD_REGISTER (node->decl))
     return NULL;
 
   sem_variable *v = new sem_variable (node, 0, stack);
@@ -1686,8 +1684,8 @@ hashval_t
 sem_variable::get_hash (void)
 {
   if (hash)
-    return hash;
 
+    return hash;
   /* All WPA streamed in symbols should have their hashes computed at compile
      time.  At this point, the constructor may not be in memory at all.
      DECL_INITIAL (decl) would be error_mark_node in that case.  */
@@ -2155,7 +2153,14 @@ sem_item_optimizer::filter_removed_items
 	  if (!flag_ipa_icf_variables)
 	    remove_item (item);
 	  else
-	    filtered.safe_push (item);
+	    {
+	      /* Filter out non-readonly variables.  */
+	      tree decl = item->decl;
+	      if (TREE_READONLY (decl))
+		filtered.safe_push (item);
+	      else
+		remove_item (item);
+	    }
         }
     }
 
Index: ipa-icf.h
===================================================================
--- ipa-icf.h	(revision 221096)
+++ ipa-icf.h	(working copy)
@@ -393,7 +393,6 @@ public:
   inline virtual void init (void)
   {
     decl = get_node ()->decl;
-    ctor = ctor_for_folding (decl);
   }
 
   virtual hashval_t get_hash (void);
@@ -415,9 +414,6 @@ public:
   /* Parser function that visits a varpool NODE.  */
   static sem_variable *parse (varpool_node *node, bitmap_obstack *stack);
 
-  /* Variable constructor.  */
-  tree ctor;
-
 private:
   /* Iterates though a constructor and identifies tree references
      we are interested in semantic function equality.  */
Index: testsuite/gcc.dg/ipa/ipa-icf-35.c
===================================================================
--- testsuite/gcc.dg/ipa/ipa-icf-35.c	(revision 0)
+++ testsuite/gcc.dg/ipa/ipa-icf-35.c	(revision 0)
@@ -0,0 +1,31 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-icf"  } */
+
+void f1()
+{
+}
+
+void f2()
+{
+}
+
+static void (*a)(void)=&f1;
+static void (*b)(void)=&f1;
+static void (*c)(void)=&f2;
+static void (*d)(void)=&f2;
+
+int main()
+{
+  a();
+  b();
+  c();
+  d();
+
+  return 0;
+}
+
+/* { dg-final { scan-ipa-dump "Equal symbols: 3" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:f2->f1" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:d->c" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:b->a" "icf"  } } */
+/* { dg-final { cleanup-ipa-dump "icf" } } */
Index: testsuite/gcc.dg/ipa/ipa-icf-36.c
===================================================================
--- testsuite/gcc.dg/ipa/ipa-icf-36.c	(revision 0)
+++ testsuite/gcc.dg/ipa/ipa-icf-36.c	(revision 0)
@@ -0,0 +1,36 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-icf -fmerge-all-constants"  } */
+static int a;
+static int b;
+static const int c = 2;
+static const int d = 2;
+static char * e = "test";
+static char * f = "test";
+static int g[3]={1,2,3};
+static int h[3]={1,2,3};
+static const int *i=&c;
+static const int *j=&c;
+static const int *k=&d;
+int t(int tt)
+{
+  switch (tt)
+  {
+    case 1: return a;
+    case 2: return b;
+    case 3: return c;
+    case 4: return d;
+    case 5: return e[1];
+    case 6: return f[1];
+    case 7: return g[1];
+    case 8: return h[1];
+    case 9: return i[0];
+    case 10: return j[0];
+    case 11: return k[0];
+  }
+}
+/* { dg-final { scan-ipa-dump "Equal symbols: 6" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:b->a" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:d->c" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:f->e" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:h->g" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:k->i" "icf"  } } */
Index: testsuite/gcc.dg/ipa/ipa-icf-37.c
===================================================================
--- testsuite/gcc.dg/ipa/ipa-icf-37.c	(revision 0)
+++ testsuite/gcc.dg/ipa/ipa-icf-37.c	(revision 0)
@@ -0,0 +1,36 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-icf"  } */
+static int a;
+static int b;
+static const int c = 2;
+static const int d = 2;
+static char * e = "test";
+static char * f = "test";
+static int g[3]={1,2,3};
+static int h[3]={1,2,3};
+static const int *i=&c;
+static const int *j=&c;
+static const int *k=&d;
+int t(int tt)
+{
+  switch (tt)
+  {
+    case 1: return a;
+    case 2: return b;
+    case 3: return c;
+    case 4: return d;
+    case 5: return e[1];
+    case 6: return f[1];
+    case 7: return g[1];
+    case 8: return h[1];
+    case 9: return i[0];
+    case 10: return j[0];
+    case 11: return k[0];
+  }
+}
+/* { dg-final { scan-ipa-dump "Equal symbols: 5" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:b->a" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:d->c" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:f->e" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:h->g" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Semantic equality hit:j->i" "icf"  } } */

Re: [PATCH] ICF: move readonly decision for variables to the right place

[Martin Liška]

On 03/02/2015 08:53 AM, Jan Hubicka wrote:
> Hi,
> this is a variant of patch I commited.
> It takes care to load the constructor to memory in sem_variable::equals
> and donot touch it earlier.  I also made sem_variable::parse to skip volatile
> and reigster variables.
>
> Bootstrapped/regtested x86_64-linux, lto-bootstrapped with -fmerge-all-constants
> comitted.
>
> Honza

Thank you Honza for patch improvement and new test cases you added.

Martin

>
> 2015-02-28  Martin Liska  <mliska@suse.cz>
> 	    Jan Hubicka   <hubicka@ucw.cz>
>
> 	* ipa-icf.c (sem_variable::equals): Improve debug output;
> 	get variable constructor.
> 	(sem_variable::parse): Do not filter out too early; give up on
> 	volatile and register vars.
> 	(sem_item_optimizer::filter_removed_items): Filter out nonreadonly
> 	variables.
> 	* ipa-icf.h (sem_variable::init): Do not set ctor.
> 	(sem_variable::ctor): Remove.
>
> gcc/testsuite/ChangeLog:
>
> 2015-02-28  Martin Liska  <mliska@suse.cz>
> 	    Jan Hubicka   <hubicka@ucw.cz>
>
> 	* gcc.dg/ipa/ipa-icf-35.c: New test.
> 	* gcc.dg/ipa/ipa-icf-36.c: New test.
> 	* gcc.dg/ipa/ipa-icf-37.c: New test.
> Index: ipa-icf.c
> ===================================================================
> --- ipa-icf.c	(revision 221096)
> +++ ipa-icf.c	(working copy)
> @@ -1448,18 +1452,29 @@ sem_variable::equals_wpa (sem_item *item
>
>   /* Returns true if the item equals to ITEM given as argument.  */
>
> +/* Returns true if the item equals to ITEM given as argument.  */
> +
>   bool
>   sem_variable::equals (sem_item *item,
> -		      hash_map <symtab_node *, sem_item *> & ARG_UNUSED (ignored_nodes))
> +		      hash_map <symtab_node *, sem_item *> &)
>   {
>     gcc_assert (item->type == VAR);
> +  bool ret;
>
> -  sem_variable *v = static_cast<sem_variable *>(item);
> -
> -  if (!ctor || !v->ctor)
> -    return return_false_with_msg ("ctor is missing for semantic variable");
> +  if (DECL_INITIAL (decl) == error_mark_node && in_lto_p)
> +    dyn_cast <varpool_node *>(node)->get_constructor ();
> +  if (DECL_INITIAL (item->decl) == error_mark_node && in_lto_p)
> +    dyn_cast <varpool_node *>(item->node)->get_constructor ();
> +
> +  ret = sem_variable::equals (DECL_INITIAL (decl),
> +			      DECL_INITIAL (item->node->decl));
> +  if (dump_file && (dump_flags & TDF_DETAILS))
> +    fprintf (dump_file,
> +	     "Equals called for vars:%s:%s (%u:%u) (%s:%s) with result: %s\n\n",
> +	     name(), item->name (), node->order, item->node->order, asm_name (),
> +	     item->asm_name (), ret ? "true" : "false");
>
> -  return sem_variable::equals (ctor, v->ctor);
> +  return ret;
>   }
>
>   /* Compares trees T1 and T2 for semantic equality.  */
> @@ -1653,24 +1668,7 @@ sem_variable::equals (tree t1, tree t2)
>   sem_variable *
>   sem_variable::parse (varpool_node *node, bitmap_obstack *stack)
>   {
> -  tree decl = node->decl;
> -
> -  if (node->alias)
> -    return NULL;
> -
> -  bool readonly = TYPE_P (decl) ? TYPE_READONLY (decl) : TREE_READONLY (decl);
> -  if (!readonly)
> -    return NULL;
> -
> -  bool can_handle = DECL_VIRTUAL_P (decl)
> -		    || flag_merge_constants >= 2
> -		    || (!TREE_ADDRESSABLE (decl) && !node->externally_visible);
> -
> -  if (!can_handle || DECL_EXTERNAL (decl))
> -    return NULL;
> -
> -  tree ctor = ctor_for_folding (decl);
> -  if (!ctor)
> +  if (TREE_THIS_VOLATILE (node->decl) || DECL_HARD_REGISTER (node->decl))
>       return NULL;
>
>     sem_variable *v = new sem_variable (node, 0, stack);
> @@ -1686,8 +1684,8 @@ hashval_t
>   sem_variable::get_hash (void)
>   {
>     if (hash)
> -    return hash;
>
> +    return hash;
>     /* All WPA streamed in symbols should have their hashes computed at compile
>        time.  At this point, the constructor may not be in memory at all.
>        DECL_INITIAL (decl) would be error_mark_node in that case.  */
> @@ -2155,7 +2153,14 @@ sem_item_optimizer::filter_removed_items
>   	  if (!flag_ipa_icf_variables)
>   	    remove_item (item);
>   	  else
> -	    filtered.safe_push (item);
> +	    {
> +	      /* Filter out non-readonly variables.  */
> +	      tree decl = item->decl;
> +	      if (TREE_READONLY (decl))
> +		filtered.safe_push (item);
> +	      else
> +		remove_item (item);
> +	    }
>           }
>       }
>
> Index: ipa-icf.h
> ===================================================================
> --- ipa-icf.h	(revision 221096)
> +++ ipa-icf.h	(working copy)
> @@ -393,7 +393,6 @@ public:
>     inline virtual void init (void)
>     {
>       decl = get_node ()->decl;
> -    ctor = ctor_for_folding (decl);
>     }
>
>     virtual hashval_t get_hash (void);
> @@ -415,9 +414,6 @@ public:
>     /* Parser function that visits a varpool NODE.  */
>     static sem_variable *parse (varpool_node *node, bitmap_obstack *stack);
>
> -  /* Variable constructor.  */
> -  tree ctor;
> -
>   private:
>     /* Iterates though a constructor and identifies tree references
>        we are interested in semantic function equality.  */
> Index: testsuite/gcc.dg/ipa/ipa-icf-35.c
> ===================================================================
> --- testsuite/gcc.dg/ipa/ipa-icf-35.c	(revision 0)
> +++ testsuite/gcc.dg/ipa/ipa-icf-35.c	(revision 0)
> @@ -0,0 +1,31 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-ipa-icf"  } */
> +
> +void f1()
> +{
> +}
> +
> +void f2()
> +{
> +}
> +
> +static void (*a)(void)=&f1;
> +static void (*b)(void)=&f1;
> +static void (*c)(void)=&f2;
> +static void (*d)(void)=&f2;
> +
> +int main()
> +{
> +  a();
> +  b();
> +  c();
> +  d();
> +
> +  return 0;
> +}
> +
> +/* { dg-final { scan-ipa-dump "Equal symbols: 3" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:f2->f1" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:d->c" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:b->a" "icf"  } } */
> +/* { dg-final { cleanup-ipa-dump "icf" } } */
> Index: testsuite/gcc.dg/ipa/ipa-icf-36.c
> ===================================================================
> --- testsuite/gcc.dg/ipa/ipa-icf-36.c	(revision 0)
> +++ testsuite/gcc.dg/ipa/ipa-icf-36.c	(revision 0)
> @@ -0,0 +1,36 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-ipa-icf -fmerge-all-constants"  } */
> +static int a;
> +static int b;
> +static const int c = 2;
> +static const int d = 2;
> +static char * e = "test";
> +static char * f = "test";
> +static int g[3]={1,2,3};
> +static int h[3]={1,2,3};
> +static const int *i=&c;
> +static const int *j=&c;
> +static const int *k=&d;
> +int t(int tt)
> +{
> +  switch (tt)
> +  {
> +    case 1: return a;
> +    case 2: return b;
> +    case 3: return c;
> +    case 4: return d;
> +    case 5: return e[1];
> +    case 6: return f[1];
> +    case 7: return g[1];
> +    case 8: return h[1];
> +    case 9: return i[0];
> +    case 10: return j[0];
> +    case 11: return k[0];
> +  }
> +}
> +/* { dg-final { scan-ipa-dump "Equal symbols: 6" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:b->a" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:d->c" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:f->e" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:h->g" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:k->i" "icf"  } } */
> Index: testsuite/gcc.dg/ipa/ipa-icf-37.c
> ===================================================================
> --- testsuite/gcc.dg/ipa/ipa-icf-37.c	(revision 0)
> +++ testsuite/gcc.dg/ipa/ipa-icf-37.c	(revision 0)
> @@ -0,0 +1,36 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-ipa-icf"  } */
> +static int a;
> +static int b;
> +static const int c = 2;
> +static const int d = 2;
> +static char * e = "test";
> +static char * f = "test";
> +static int g[3]={1,2,3};
> +static int h[3]={1,2,3};
> +static const int *i=&c;
> +static const int *j=&c;
> +static const int *k=&d;
> +int t(int tt)
> +{
> +  switch (tt)
> +  {
> +    case 1: return a;
> +    case 2: return b;
> +    case 3: return c;
> +    case 4: return d;
> +    case 5: return e[1];
> +    case 6: return f[1];
> +    case 7: return g[1];
> +    case 8: return h[1];
> +    case 9: return i[0];
> +    case 10: return j[0];
> +    case 11: return k[0];
> +  }
> +}
> +/* { dg-final { scan-ipa-dump "Equal symbols: 5" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:b->a" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:d->c" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:f->e" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:h->g" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Semantic equality hit:j->i" "icf"  } } */
>

Re: [PATCH] ICF: move readonly decision for variables to the right place

[H.J. Lu]

On Sun, Mar 1, 2015 at 11:53 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> this is a variant of patch I commited.
> It takes care to load the constructor to memory in sem_variable::equals
> and donot touch it earlier.  I also made sem_variable::parse to skip volatile
> and reigster variables.
>
> Bootstrapped/regtested x86_64-linux, lto-bootstrapped with -fmerge-all-constants
> comitted.
>
> Honza
>
> 2015-02-28  Martin Liska  <mliska@suse.cz>
>             Jan Hubicka   <hubicka@ucw.cz>
>
>         * ipa-icf.c (sem_variable::equals): Improve debug output;
>         get variable constructor.
>         (sem_variable::parse): Do not filter out too early; give up on
>         volatile and register vars.
>         (sem_item_optimizer::filter_removed_items): Filter out nonreadonly
>         variables.
>         * ipa-icf.h (sem_variable::init): Do not set ctor.
>         (sem_variable::ctor): Remove.
>

This caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65334

-- 
H.J.

Re: [PATCH] ICF: move readonly decision for variables to the right place

[Jan Hubicka]

> 
> This caused:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65334

This is another alias issue exposed by ICF.  Vectorizer attempts to increase an alignment of alias
by modifying its DECL_ALIGN, but it doesnot really achieve it - when outputting the definition we
use DECL_ALIGN of the ultimate_alias_target.

Frankly, increasing alias target is not safe thing to do: alias may be static,
while target interposable and we do not want code touching actual target to
assume greater alignment.  I suppose only way to get this right is to make
varasm to walk all aliases and figure largest alignment assumed.  I am workign
on a patch.

Honza
> 
> -- 
> H.J.