[PATCH] Fix PR ipa/65318

[PATCH] Fix PR ipa/65318

[Martin Liška]

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

Hello.

This is patch that prevents merge operation for ICF on variables types which are not compatible.
Regression tests were run on x86_64-linux-pc.

Ready for trunk?
Thanks,
Martin

[-- Attachment #2: 0001-Fix-PR-ipa-65318.patch --]
[-- Type: text/x-patch, Size: 1833 bytes --]

From b92ec230162b99ff11d4e5688f63ae978e75af12 Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Thu, 5 Mar 2015 13:41:07 +0100
Subject: [PATCH] Fix PR ipa/65318.

gcc/ChangeLog:

2015-03-05  Martin Liska  <mliska@suse.cz>

	PR ipa/65318
	* ipa-icf.c (sem_variable::equals): Compare variable types.

gcc/testsuite/ChangeLog:

2015-03-05  Martin Liska  <mliska@suse.cz>

	* gcc.dg/ipa/PR65318.c: New test.
---
 gcc/ipa-icf.c                      |  5 +++++
 gcc/testsuite/gcc.dg/ipa/PR65318.c | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/PR65318.c

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index c55a09f..1752e67 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1501,6 +1501,11 @@ sem_variable::equals (sem_item *item,
   if (DECL_INITIAL (item->decl) == error_mark_node && in_lto_p)
     dyn_cast <varpool_node *>(item->node)->get_constructor ();
 
+  /* As seen in PR ipa/65303 we have to compare variable's types.  */
+  if (!func_checker::compatible_types_p(TREE_TYPE (decl),
+					TREE_TYPE (item->decl)))
+    return return_false_with_msg ("variable types are different");
+
   ret = sem_variable::equals (DECL_INITIAL (decl),
 			      DECL_INITIAL (item->node->decl));
   if (dump_file && (dump_flags & TDF_DETAILS))
diff --git a/gcc/testsuite/gcc.dg/ipa/PR65318.c b/gcc/testsuite/gcc.dg/ipa/PR65318.c
new file mode 100644
index 0000000..f23b3a2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/PR65318.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-icf-details"  } */
+
+static short a = 0;
+short b = -1;
+static unsigned short c = 0;
+
+int
+main ()
+{
+  if (a <= b)
+   return 1;
+
+  return 0;
+}
+
+/* { dg-final { scan-ipa-dump "Equal symbols: 0" "icf"  } } */
+/* { dg-final { cleanup-ipa-dump "icf" } } */
-- 
2.1.2

Re: [PATCH] Fix PR ipa/65318

[Marek Polacek]

On Thu, Mar 05, 2015 at 02:53:44PM +0100, Martin Liška wrote:
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -1501,6 +1501,11 @@ sem_variable::equals (sem_item *item,
>    if (DECL_INITIAL (item->decl) == error_mark_node && in_lto_p)
>      dyn_cast <varpool_node *>(item->node)->get_constructor ();
>  
> +  /* As seen in PR ipa/65303 we have to compare variable's types.  */

"variables"?

> +  if (!func_checker::compatible_types_p(TREE_TYPE (decl),

Missing space before paren.

> +					TREE_TYPE (item->decl)))
> +    return return_false_with_msg ("variable types are different");

Here "variables" as well?

> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/PR65318.c

Why PR* and not pr*, which is common?

	Marek

Re: [PATCH] Fix PR ipa/65318

[Martin Liška]

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

On 03/05/2015 03:29 PM, Marek Polacek wrote:
> On Thu, Mar 05, 2015 at 02:53:44PM +0100, Martin Liška wrote:
>> --- a/gcc/ipa-icf.c
>> +++ b/gcc/ipa-icf.c
>> @@ -1501,6 +1501,11 @@ sem_variable::equals (sem_item *item,
>>     if (DECL_INITIAL (item->decl) == error_mark_node && in_lto_p)
>>       dyn_cast <varpool_node *>(item->node)->get_constructor ();
>>
>> +  /* As seen in PR ipa/65303 we have to compare variable's types.  */
>
> "variables"?
>
>> +  if (!func_checker::compatible_types_p(TREE_TYPE (decl),
>
> Missing space before paren.
>
>> +					TREE_TYPE (item->decl)))
>> +    return return_false_with_msg ("variable types are different");
>
> Here "variables" as well?
>
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/ipa/PR65318.c
>
> Why PR* and not pr*, which is common?
>
> 	Marek
>

You are right, pr* is more common.
Other nits are fixed in updated version of patch.

Thanks,
Martin

[-- Attachment #2: 0001-Fix-PR-ipa-65318.patch --]
[-- Type: text/x-patch, Size: 1836 bytes --]

From 3f35d9ec57880409cde384bb7b9e8dbaae5231ef Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Thu, 5 Mar 2015 13:41:07 +0100
Subject: [PATCH] Fix PR ipa/65318.

gcc/ChangeLog:

2015-03-05  Martin Liska  <mliska@suse.cz>

	PR ipa/65318
	* ipa-icf.c (sem_variable::equals): Compare variables types.

gcc/testsuite/ChangeLog:

2015-03-05  Martin Liska  <mliska@suse.cz>

	* gcc.dg/ipa/pr65318.c: New test.
---
 gcc/ipa-icf.c                      |  5 +++++
 gcc/testsuite/gcc.dg/ipa/pr65318.c | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr65318.c

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index c55a09f..a7f19d6 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1501,6 +1501,11 @@ sem_variable::equals (sem_item *item,
   if (DECL_INITIAL (item->decl) == error_mark_node && in_lto_p)
     dyn_cast <varpool_node *>(item->node)->get_constructor ();
 
+  /* As seen in PR ipa/65303 we have to compare variables types.  */
+  if (!func_checker::compatible_types_p (TREE_TYPE (decl),
+					 TREE_TYPE (item->decl)))
+    return return_false_with_msg ("variables types are different");
+
   ret = sem_variable::equals (DECL_INITIAL (decl),
 			      DECL_INITIAL (item->node->decl));
   if (dump_file && (dump_flags & TDF_DETAILS))
diff --git a/gcc/testsuite/gcc.dg/ipa/pr65318.c b/gcc/testsuite/gcc.dg/ipa/pr65318.c
new file mode 100644
index 0000000..f23b3a2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr65318.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-icf-details"  } */
+
+static short a = 0;
+short b = -1;
+static unsigned short c = 0;
+
+int
+main ()
+{
+  if (a <= b)
+   return 1;
+
+  return 0;
+}
+
+/* { dg-final { scan-ipa-dump "Equal symbols: 0" "icf"  } } */
+/* { dg-final { cleanup-ipa-dump "icf" } } */
-- 
2.1.2

Re: [PATCH] Fix PR ipa/65318

[Jeff Law]

On 03/05/15 07:37, Martin Liška wrote:
>
>
>  From 3f35d9ec57880409cde384bb7b9e8dbaae5231ef Mon Sep 17 00:00:00 2001
> From: mliska<mliska@suse.cz>
> Date: Thu, 5 Mar 2015 13:41:07 +0100
> Subject: [PATCH] Fix PR ipa/65318.
>
> gcc/ChangeLog:
>
> 2015-03-05  Martin Liska<mliska@suse.cz>
>
> 	PR ipa/65318
> 	* ipa-icf.c (sem_variable::equals): Compare variables types.
>
> gcc/testsuite/ChangeLog:
>
> 2015-03-05  Martin Liska<mliska@suse.cz>
>
> 	* gcc.dg/ipa/pr65318.c: New test.
OK.
Jeff

Re: [PATCH] Fix PR ipa/65318

[Jan Hubicka]

> gcc/ChangeLog:
> 
> 2015-03-05  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/65318
> 	* ipa-icf.c (sem_variable::equals): Compare variables types.
> 
> gcc/testsuite/ChangeLog:
> 
> 2015-03-05  Martin Liska  <mliska@suse.cz>
> 
> 	* gcc.dg/ipa/pr65318.c: New test.

OK,
Honza

Re: [PATCH] Fix PR ipa/65318

[Jan Hubicka]

> > gcc/ChangeLog:
> > 
> > 2015-03-05  Martin Liska  <mliska@suse.cz>
> > 
> > 	PR ipa/65318
> > 	* ipa-icf.c (sem_variable::equals): Compare variables types.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 2015-03-05  Martin Liska  <mliska@suse.cz>
> > 
> > 	* gcc.dg/ipa/pr65318.c: New test.
> 
> OK,
Though actually I think it is papering over folding issue - probably we do want
to imply VIEW_CONVERT_EXPR when type of alias and type of variable does not match.

Honza
> Honza

Re: [PATCH] Fix PR ipa/65318

[Jan Hubicka]

Hi,
this patch sovles the incorrect folding. The very same unification (ignoring
signedness by checking that memory representation is the same) is done by
constant pool.

Some of the other uses of ctor_for_folding therefore already uses
VIEW_CONVERT_EXPR, I suppose as a partial fix for past bugs. This particular
case is handled by get_symbol_constant_value that does not VCE. Maybe
we usually don't drop scalar constant to constant pool that often, so this
did not show up.

Attached is non-ICF testcase. It is bit questionable if we consider this to be
valid, but it is better to be safe than sorry.  Mixing signed/unsigned may be
more common with LTO.

Bootstrap/regtest running in x86_64-linux, seems sane?

Honza

static short a __attribute__ ((alias ("c")));
short b = -1;
static unsigned short c = 0;

int
main ()
{
  if (a <= b)
    return 1;
  return 0;
}


	* gimple-fold.c (get_symbol_constant_value): Convert to symbol type.

Index: gimple-fold.c
===================================================================
--- gimple-fold.c	(revision 221170)
+++ gimple-fold.c	(working copy)
@@ -263,7 +263,16 @@ get_symbol_constant_value (tree sym)
 	{
 	  val = canonicalize_constructor_val (unshare_expr (val), sym);
 	  if (val && is_gimple_min_invariant (val))
-	    return val;
+	    {
+              if (!useless_type_conversion_p (TREE_TYPE (sym), TREE_TYPE (val)))
+		{
+		  if (operand_equal_p (TYPE_SIZE (TREE_TYPE (sym)),
+				       TYPE_SIZE (TREE_TYPE (val)), 0))
+		    return NULL_TREE;
+		  val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (sym), val);
+		}
+	      return val;
+	    }
 	  else
 	    return NULL_TREE;
 	}

Re: [PATCH] Fix PR ipa/65318

[Richard Biener]

On March 5, 2015 7:08:16 PM CET, Jan Hubicka <hubicka@ucw.cz> wrote:
>Hi,
>this patch sovles the incorrect folding. The very same unification
>(ignoring
>signedness by checking that memory representation is the same) is done
>by
>constant pool.
>
>Some of the other uses of ctor_for_folding therefore already uses
>VIEW_CONVERT_EXPR, I suppose as a partial fix for past bugs. This
>particular
>case is handled by get_symbol_constant_value that does not VCE. Maybe
>we usually don't drop scalar constant to constant pool that often, so
>this
>did not show up.
>
>Attached is non-ICF testcase. It is bit questionable if we consider
>this to be
>valid, but it is better to be safe than sorry.  Mixing signed/unsigned
>may be
>more common with LTO.

With LTO we wrap all memory accesses in MEM_REFs during streaming which preserves the original types (and thus act as view-conversion if necessary). Without LTO the aliases should provide the same.

Richard.

>Bootstrap/regtest running in x86_64-linux, seems sane?
>
>Honza
>
>static short a __attribute__ ((alias ("c")));
>short b = -1;
>static unsigned short c = 0;
>
>int
>main ()
>{
>  if (a <= b)
>    return 1;
>  return 0;
>}
>
>
>	* gimple-fold.c (get_symbol_constant_value): Convert to symbol type.
>
>Index: gimple-fold.c
>===================================================================
>--- gimple-fold.c	(revision 221170)
>+++ gimple-fold.c	(working copy)
>@@ -263,7 +263,16 @@ get_symbol_constant_value (tree sym)
> 	{
> 	  val = canonicalize_constructor_val (unshare_expr (val), sym);
> 	  if (val && is_gimple_min_invariant (val))
>-	    return val;
>+	    {
>+              if (!useless_type_conversion_p (TREE_TYPE (sym),
>TREE_TYPE (val)))
>+		{
>+		  if (operand_equal_p (TYPE_SIZE (TREE_TYPE (sym)),
>+				       TYPE_SIZE (TREE_TYPE (val)), 0))
>+		    return NULL_TREE;
>+		  val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (sym), val);
>+		}
>+	      return val;
>+	    }
> 	  else
> 	    return NULL_TREE;
> 	}

Re: [PATCH] Fix PR ipa/65318

[Richard Biener]

On March 5, 2015 7:08:16 PM CET, Jan Hubicka <hubicka@ucw.cz> wrote:
>Hi,
>this patch sovles the incorrect folding. The very same unification
>(ignoring
>signedness by checking that memory representation is the same) is done
>by
>constant pool.
>
>Some of the other uses of ctor_for_folding therefore already uses
>VIEW_CONVERT_EXPR, I suppose as a partial fix for past bugs. This
>particular
>case is handled by get_symbol_constant_value that does not VCE. Maybe
>we usually don't drop scalar constant to constant pool that often, so
>this
>did not show up.
>
>Attached is non-ICF testcase. It is bit questionable if we consider
>this to be
>valid, but it is better to be safe than sorry.  Mixing signed/unsigned
>may be
>more common with LTO.
>
>Bootstrap/regtest running in x86_64-linux, seems sane?
>
>Honza
>
>static short a __attribute__ ((alias ("c")));
>short b = -1;
>static unsigned short c = 0;
>
>int
>main ()
>{
>  if (a <= b)
>    return 1;
>  return 0;
>}
>
>
>	* gimple-fold.c (get_symbol_constant_value): Convert to symbol type.
>
>Index: gimple-fold.c
>===================================================================
>--- gimple-fold.c	(revision 221170)
>+++ gimple-fold.c	(working copy)
>@@ -263,7 +263,16 @@ get_symbol_constant_value (tree sym)
> 	{
> 	  val = canonicalize_constructor_val (unshare_expr (val), sym);
> 	  if (val && is_gimple_min_invariant (val))
>-	    return val;
>+	    {
>+              if (!useless_type_conversion_p (TREE_TYPE (sym),
>TREE_TYPE (val)))
>+		{
>+		  if (operand_equal_p (TYPE_SIZE (TREE_TYPE (sym)),
>+				       TYPE_SIZE (TREE_TYPE (val)), 0))
>+		    return NULL_TREE;

And no, I don't think this is sane.  Callers need to handle mismatches IIRC.

>+		  val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (sym), val);
>+		}
>+	      return val;
>+	    }
> 	  else
> 	    return NULL_TREE;
> 	}

Re: [PATCH] Fix PR ipa/65318

[Jan Hubicka]

> >Index: gimple-fold.c
> >===================================================================
> >--- gimple-fold.c	(revision 221170)
> >+++ gimple-fold.c	(working copy)
> >@@ -263,7 +263,16 @@ get_symbol_constant_value (tree sym)
> > 	{
> > 	  val = canonicalize_constructor_val (unshare_expr (val), sym);
> > 	  if (val && is_gimple_min_invariant (val))
> >-	    return val;
> >+	    {
> >+              if (!useless_type_conversion_p (TREE_TYPE (sym),
> >TREE_TYPE (val)))
> >+		{
> >+		  if (operand_equal_p (TYPE_SIZE (TREE_TYPE (sym)),
> >+				       TYPE_SIZE (TREE_TYPE (val)), 0))
> >+		    return NULL_TREE;
> 
> And no, I don't think this is sane.  Callers need to handle mismatches IIRC.

OK, I am little bit confused about your MEM_REF suggestion. So you mean that
MEM_REF should be added around all references to symbol that is an alias?
Where it is done?  Is there a reason why we do not add MEM_REF always?  I would
like to keep optimization passes (like ipa-visibility or ICF) to turn symbol
into an alias without having to update underlying IL.

Concerning callers handling mismatches, the VIEW_CONVERT_EXPR thing seems valid
thing to do for all uses except for fold_const_aggregate_ref_1. So perhaps
we can just inline rest of get_symbol_constant_value in there and document that
get_symbol_constant_value returns value in correct type.

Or am I missing something obvious?

Thanks!
Honza
> 
> >+		  val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (sym), val);
> >+		}
> >+	      return val;
> >+	    }
> > 	  else
> > 	    return NULL_TREE;
> > 	}
> 

Re: [PATCH] Fix PR ipa/65318

[Richard Biener]

On Thu, Mar 5, 2015 at 7:38 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >Index: gimple-fold.c
>> >===================================================================
>> >--- gimple-fold.c    (revision 221170)
>> >+++ gimple-fold.c    (working copy)
>> >@@ -263,7 +263,16 @@ get_symbol_constant_value (tree sym)
>> >     {
>> >       val = canonicalize_constructor_val (unshare_expr (val), sym);
>> >       if (val && is_gimple_min_invariant (val))
>> >-        return val;
>> >+        {
>> >+              if (!useless_type_conversion_p (TREE_TYPE (sym),
>> >TREE_TYPE (val)))
>> >+            {
>> >+              if (operand_equal_p (TYPE_SIZE (TREE_TYPE (sym)),
>> >+                                   TYPE_SIZE (TREE_TYPE (val)), 0))
>> >+                return NULL_TREE;
>>
>> And no, I don't think this is sane.  Callers need to handle mismatches IIRC.
>
> OK, I am little bit confused about your MEM_REF suggestion. So you mean that
> MEM_REF should be added around all references to symbol that is an alias?
> Where it is done?  Is there a reason why we do not add MEM_REF always?  I would
> like to keep optimization passes (like ipa-visibility or ICF) to turn symbol
> into an alias without having to update underlying IL.

Yes - but I said that having an alias should have the same effect as
the MEM_REF wrapping we do in LTO (to not barf on stmt verification
if symbol merging merges an int and a float for example).

>
> Concerning callers handling mismatches, the VIEW_CONVERT_EXPR thing seems valid
> thing to do for all uses except for fold_const_aggregate_ref_1. So perhaps
> we can just inline rest of get_symbol_constant_value in there and document that
> get_symbol_constant_value returns value in correct type.
>
> Or am I missing something obvious?

Yeah, that looks good.  Note that we can as well change all callers
of get_symbol_constant_value to use fold_const_aggregate_ref, no?
So reduce the number of APIs.

Richard.

>
> Thanks!
> Honza
>>
>> >+              val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (sym), val);
>> >+            }
>> >+          return val;
>> >+        }
>> >       else
>> >         return NULL_TREE;
>> >     }
>>

Re: [PATCH] Fix PR ipa/65318

[Jan Hubicka]

> 
> Yes - but I said that having an alias should have the same effect as
> the MEM_REF wrapping we do in LTO (to not barf on stmt verification
> if symbol merging merges an int and a float for example).

Yep, though alias is bit different from LTO - in LTO we replace the decl in place,
while alias still has its oriignal type only its constructor is not necessarily
cooperating. (We sort of do that with -fmerge-constants for CONST_DECL and in constant
pool vairables for ages)
> 
> >
> > Concerning callers handling mismatches, the VIEW_CONVERT_EXPR thing seems valid
> > thing to do for all uses except for fold_const_aggregate_ref_1. So perhaps
> > we can just inline rest of get_symbol_constant_value in there and document that
> > get_symbol_constant_value returns value in correct type.
> >
> > Or am I missing something obvious?
> 
> Yeah, that looks good.  Note that we can as well change all callers
> of get_symbol_constant_value to use fold_const_aggregate_ref, no?
> So reduce the number of APIs.

Yes, that seems like good idea.  As I recall, we used to have get_symbol_constant_value
first and introduced aggregate folding later.

Honza
> 
> Richard.
> 
> >
> > Thanks!
> > Honza
> >>
> >> >+              val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (sym), val);
> >> >+            }
> >> >+          return val;
> >> >+        }
> >> >       else
> >> >         return NULL_TREE;
> >> >     }
> >>