public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* Drop CONSTRUCTOR comparsion from ipa-icf-gimple
@ 2015-10-16  3:12 Jan Hubicka
  2015-10-16  8:48 ` Richard Biener
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Hubicka @ 2015-10-16  3:12 UTC (permalink / raw)
  To: gcc-patches

Hi,
as Richard noticed in my port of the code to operand_equal_p, the checking of
CONSTURCTOR in ipa-icf-gimple is incomplete missing the index checks.
It is also unnecesary since non-empty ctors does not happen as gimple
operands.  This patch thus removes the unnecesary code.

Bootstrapped/regtested x86_64-linux, comitted.

Honza

	* ipa-icf-gimple.c (func_checker::compare_operand): Compare only
	empty constructors.
Index: ipa-icf-gimple.c
===================================================================
--- ipa-icf-gimple.c	(revision 228851)
+++ ipa-icf-gimple.c	(working copy)
@@ -415,20 +415,9 @@ func_checker::compare_operand (tree t1,
   switch (TREE_CODE (t1))
     {
     case CONSTRUCTOR:
-      {
-	unsigned length1 = vec_safe_length (CONSTRUCTOR_ELTS (t1));
-	unsigned length2 = vec_safe_length (CONSTRUCTOR_ELTS (t2));
-
-	if (length1 != length2)
-	  return return_false ();
-
-	for (unsigned i = 0; i < length1; i++)
-	  if (!compare_operand (CONSTRUCTOR_ELT (t1, i)->value,
-				CONSTRUCTOR_ELT (t2, i)->value))
-	    return return_false();
-
-	return true;
-      }
+      gcc_assert (!vec_safe_length (CONSTRUCTOR_ELTS (t1))
+		  && !vec_safe_length (CONSTRUCTOR_ELTS (t2)));
+      return true;
     case ARRAY_REF:
     case ARRAY_RANGE_REF:
       /* First argument is the array, second is the index.  */

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Drop CONSTRUCTOR comparsion from ipa-icf-gimple
  2015-10-16  3:12 Drop CONSTRUCTOR comparsion from ipa-icf-gimple Jan Hubicka
@ 2015-10-16  8:48 ` Richard Biener
  2015-10-16 10:54   ` H.J. Lu
  0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2015-10-16  8:48 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: GCC Patches

On Fri, Oct 16, 2015 at 5:12 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> as Richard noticed in my port of the code to operand_equal_p, the checking of
> CONSTURCTOR in ipa-icf-gimple is incomplete missing the index checks.
> It is also unnecesary since non-empty ctors does not happen as gimple
> operands.  This patch thus removes the unnecesary code.

Err - they do happen, for vector constructors.  Just empty constructors
are not allowed for vector constructors - vector constructors are required
to have elements in proper order and none left out.

Sorry for misleading you.

> Bootstrapped/regtested x86_64-linux, comitted.

this will definitely ICE ...

Richard.

> Honza
>
>         * ipa-icf-gimple.c (func_checker::compare_operand): Compare only
>         empty constructors.
> Index: ipa-icf-gimple.c
> ===================================================================
> --- ipa-icf-gimple.c    (revision 228851)
> +++ ipa-icf-gimple.c    (working copy)
> @@ -415,20 +415,9 @@ func_checker::compare_operand (tree t1,
>    switch (TREE_CODE (t1))
>      {
>      case CONSTRUCTOR:
> -      {
> -       unsigned length1 = vec_safe_length (CONSTRUCTOR_ELTS (t1));
> -       unsigned length2 = vec_safe_length (CONSTRUCTOR_ELTS (t2));
> -
> -       if (length1 != length2)
> -         return return_false ();
> -
> -       for (unsigned i = 0; i < length1; i++)
> -         if (!compare_operand (CONSTRUCTOR_ELT (t1, i)->value,
> -                               CONSTRUCTOR_ELT (t2, i)->value))
> -           return return_false();
> -
> -       return true;
> -      }
> +      gcc_assert (!vec_safe_length (CONSTRUCTOR_ELTS (t1))
> +                 && !vec_safe_length (CONSTRUCTOR_ELTS (t2)));
> +      return true;
>      case ARRAY_REF:
>      case ARRAY_RANGE_REF:
>        /* First argument is the array, second is the index.  */

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Drop CONSTRUCTOR comparsion from ipa-icf-gimple
  2015-10-16  8:48 ` Richard Biener
@ 2015-10-16 10:54   ` H.J. Lu
  2015-10-16 10:55     ` Jan Hubicka
  0 siblings, 1 reply; 4+ messages in thread
From: H.J. Lu @ 2015-10-16 10:54 UTC (permalink / raw)
  To: Richard Biener; +Cc: Jan Hubicka, GCC Patches

On Fri, Oct 16, 2015 at 1:46 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Fri, Oct 16, 2015 at 5:12 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Hi,
>> as Richard noticed in my port of the code to operand_equal_p, the checking of
>> CONSTURCTOR in ipa-icf-gimple is incomplete missing the index checks.
>> It is also unnecesary since non-empty ctors does not happen as gimple
>> operands.  This patch thus removes the unnecesary code.
>
> Err - they do happen, for vector constructors.  Just empty constructors
> are not allowed for vector constructors - vector constructors are required
> to have elements in proper order and none left out.
>
> Sorry for misleading you.
>
>> Bootstrapped/regtested x86_64-linux, comitted.
>
> this will definitely ICE ...
>

And it did on x86:

https://gcc.gnu.org/ml/gcc-regression/2015-10/msg00166.html

FAIL: gcc.dg/pr63914.c (internal compiler error)
FAIL: gcc.dg/pr63914.c (test for excess errors)
FAIL: gcc.target/i386/avx-1.c (internal compiler error)
FAIL: gcc.target/i386/avx-1.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v16sf-1.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v16sf-1.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v16sf-2.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v16sf-2.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v16sf-3.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v16sf-3.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v16si-1.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v16si-1.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v16si-2.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v16si-2.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v16si-3.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v16si-3.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v8df-1.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v8df-1.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v8df-2.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v8df-2.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v8df-3.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v8df-3.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v8di-1.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v8di-1.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v8di-2.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v8di-2.c (test for excess errors)
FAIL: gcc.target/i386/avx512f-set-v8di-3.c (internal compiler error)
FAIL: gcc.target/i386/avx512f-set-v8di-3.c (test for excess errors)
FAIL: gcc.target/i386/sse-13.c (internal compiler error)
FAIL: gcc.target/i386/sse-13.c (test for excess errors)
FAIL: gcc.target/i386/sse-18.c (internal compiler error)
FAIL: gcc.target/i386/sse-18.c (test for excess errors)
FAIL: gcc.target/i386/sse-19.c (internal compiler error)
FAIL: gcc.target/i386/sse-19.c (test for excess errors)
FAIL: gcc.target/i386/sse-23.c (internal compiler error)
FAIL: gcc.target/i386/sse-23.c (test for excess errors)
FAIL: gcc.target/i386/sse-24.c (internal compiler error)
FAIL: gcc.target/i386/sse-24.c (test for excess errors)
FAIL: gcc.target/i386/sse-25.c (internal compiler error)
FAIL: gcc.target/i386/sse-25.c (test for excess errors)
FAIL: gcc.target/i386/vecinit-1.c (internal compiler error)
FAIL: gcc.target/i386/vecinit-1.c (test for excess errors)
FAIL: gcc.target/i386/vecinit-2.c (internal compiler error)
FAIL: gcc.target/i386/vecinit-2.c (test for excess errors)
FAIL: gcc.target/i386/vecinit-5.c (internal compiler error)
FAIL: gcc.target/i386/vecinit-5.c (test for excess errors)
FAIL: gcc.target/i386/vecinit-6.c (internal compiler error)
FAIL: gcc.target/i386/vecinit-6.c (test for excess errors)

-- 
H.J.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Drop CONSTRUCTOR comparsion from ipa-icf-gimple
  2015-10-16 10:54   ` H.J. Lu
@ 2015-10-16 10:55     ` Jan Hubicka
  0 siblings, 0 replies; 4+ messages in thread
From: Jan Hubicka @ 2015-10-16 10:55 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Richard Biener, Jan Hubicka, GCC Patches

> On Fri, Oct 16, 2015 at 1:46 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
> > On Fri, Oct 16, 2015 at 5:12 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> Hi,
> >> as Richard noticed in my port of the code to operand_equal_p, the checking of
> >> CONSTURCTOR in ipa-icf-gimple is incomplete missing the index checks.
> >> It is also unnecesary since non-empty ctors does not happen as gimple
> >> operands.  This patch thus removes the unnecesary code.
> >
> > Err - they do happen, for vector constructors.  Just empty constructors
> > are not allowed for vector constructors - vector constructors are required
> > to have elements in proper order and none left out.
> >
> > Sorry for misleading you.
> >
> >> Bootstrapped/regtested x86_64-linux, comitted.
> >
> > this will definitely ICE ...
> >
> 
> And it did on x86:
> 
> https://gcc.gnu.org/ml/gcc-regression/2015-10/msg00166.html
> 
I am going to commit the following revert wich also adds generic testcase
as soon as testing converges.

Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog	(revision 228865)
+++ testsuite/ChangeLog	(working copy)
@@ -1,3 +1,7 @@
+2015-10-11  Jan Hubicka  <hubicka@ucw.cz>
+
+	* gcc.c-torture/compile/icfmatch.c: Add testcase
+
 2015-10-16  Paolo Carlini  <paolo.carlini@oracle.com>
 
 	PR c++/67926
Index: testsuite/gcc.c-torture/compile/icfmatch.c
===================================================================
--- testsuite/gcc.c-torture/compile/icfmatch.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/icfmatch.c	(revision 0)
@@ -0,0 +1,11 @@
+typedef char __attribute__ ((vector_size (4))) v4qi;
+void retv (int a,int b,int c,int d, v4qi *ret)
+{
+  v4qi v = { a, b , c, d };
+  *ret = v;
+}
+void retv2 (int a,int b,int c,int d, v4qi *ret)
+{
+  v4qi v = { a, b , c, d };
+  *ret = v;
+}
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 228867)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2015-10-11  Jan Hubicka  <hubicka@ucw.cz>
+
+	Revert:
+	* ipa-icf-gimple.c (func_checker::compare_operand): Compare only
+	empty constructors.
+
 2015-10-16  Richard Biener  <rguenther@suse.de>
 
 	* gimple-fold.c (gimple_fold_builtin_memory_op): Use gimple_build

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-10-16 10:55 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-16  3:12 Drop CONSTRUCTOR comparsion from ipa-icf-gimple Jan Hubicka
2015-10-16  8:48 ` Richard Biener
2015-10-16 10:54   ` H.J. Lu
2015-10-16 10:55     ` Jan Hubicka

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).