[PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[Martin Liška]

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

Hi.

The patch is about more fine comparison of loop fields that
are mentioned in the PR.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

gcc/ChangeLog:

2019-05-23  Martin Liska  <mliska@suse.cz>

	PR ipa/90555
	* ipa-icf-gimple.c (func_checker::compare_loops): New function.
	* ipa-icf-gimple.h (func_checker::compare_loops): Likewise.
	* ipa-icf.c (sem_function::equals_private): Use compare_loops.

gcc/testsuite/ChangeLog:

2019-05-23  Martin Liska  <mliska@suse.cz>

	PR ipa/90555
	* gcc.dg/ipa/pr90555.c: New test.
---
 gcc/ipa-icf-gimple.c               | 40 ++++++++++++++++++
 gcc/ipa-icf-gimple.h               |  3 ++
 gcc/ipa-icf.c                      |  3 ++
 gcc/testsuite/gcc.dg/ipa/pr90555.c | 67 ++++++++++++++++++++++++++++++
 4 files changed, 113 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr90555.c



[-- Attachment #2: 0001-Handle-loop-fields-in-IPA-ICF-PR-ipa-90555.patch --]
[-- Type: text/x-patch, Size: 4250 bytes --]

diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
index 25284936bc3..4d4e4c7c350 100644
--- a/gcc/ipa-icf-gimple.c
+++ b/gcc/ipa-icf-gimple.c
@@ -37,6 +37,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "ipa-utils.h"
 #include "tree-eh.h"
 #include "builtins.h"
+#include "cfgloop.h"
 
 #include "ipa-icf-gimple.h"
 
@@ -605,6 +606,45 @@ func_checker::compare_variable_decl (tree t1, tree t2)
   return return_with_debug (ret);
 }
 
+/* Compare significant loop information.  */
+bool
+func_checker::compare_loops (void)
+{
+  struct loop *loop;
+
+  auto_vec <struct loop *> loops1;
+  auto_vec <struct loop *> loops2;
+
+  FOR_EACH_LOOP_FN (DECL_STRUCT_FUNCTION (m_source_func_decl), loop, 0)
+    loops1.safe_push (loop);
+
+  FOR_EACH_LOOP_FN (DECL_STRUCT_FUNCTION (m_target_func_decl), loop, 0)
+    loops2.safe_push (loop);
+
+  gcc_assert (loops1.length () == loops2.length ());
+
+  for (unsigned i = 0; i < loops1.length (); i++)
+    {
+      struct loop *l1 = loops1[i];
+      struct loop *l2 = loops2[i];
+      if (l1->simdlen != l2->simdlen)
+	return return_false_with_msg ("simdlen");
+      if (l1->safelen != l2->safelen)
+	return return_false_with_msg ("safelen");
+      if (l1->can_be_parallel != l2->can_be_parallel)
+	return return_false_with_msg ("can_be_parallel");
+      if (l1->dont_vectorize != l2->dont_vectorize)
+	return return_false_with_msg ("dont_vectorize");
+      if (l1->force_vectorize != l2->force_vectorize)
+	return return_false_with_msg ("force_vectorize");
+      if (l1->unroll != l2->unroll)
+	return return_false_with_msg ("unroll");
+      if (!compare_variable_decl (l1->simduid, l2->simduid))
+	return return_false_with_msg ("simduid");
+    }
+
+  return true;
+}
 
 /* Function visits all gimple labels and creates corresponding
    mapping between basic blocks and labels.  */
diff --git a/gcc/ipa-icf-gimple.h b/gcc/ipa-icf-gimple.h
index 0035db32e66..0fb0744d912 100644
--- a/gcc/ipa-icf-gimple.h
+++ b/gcc/ipa-icf-gimple.h
@@ -226,6 +226,9 @@ public:
   /* Verifies that trees T1 and T2 do correspond.  */
   bool compare_variable_decl (tree t1, tree t2);
 
+  /* Compare significant loop information.  */
+  bool compare_loops (void);
+
   /* Return true if types are compatible for polymorphic call analysis.
      COMPARE_PTR indicates if polymorphic type comparsion should be
      done for pointers, too.  */
diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index 074181491da..ba5121f2198 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -926,6 +926,9 @@ sem_function::equals_private (sem_item *item)
     if (!compare_phi_node (bb_sorted[i]->bb, m_compared_func->bb_sorted[i]->bb))
       return return_false_with_msg ("PHI node comparison returns false");
 
+  if (!m_checker->compare_loops ())
+    return return_false_with_msg ("loops are different");
+
   return result;
 }
 
diff --git a/gcc/testsuite/gcc.dg/ipa/pr90555.c b/gcc/testsuite/gcc.dg/ipa/pr90555.c
new file mode 100644
index 00000000000..a9c751cd63c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr90555.c
@@ -0,0 +1,67 @@
+/* { dg-do compile } */
+/* { dg-options "-fopenmp-simd -O2 -mavx512f -fdump-ipa-icf" } */
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+
+#define N 1024
+int a[N];
+
+void
+test_simdlen1 (void)
+{
+  int i;
+  #pragma omp simd simdlen (4)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+void
+test_simdlen2 (void)
+{
+  int i;
+  #pragma omp simd simdlen (8)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+void
+test_safelen1 (void)
+{
+  int i;
+  #pragma omp simd safelen (4)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+void
+test_safelen2 (void)
+{
+  int i;
+  #pragma omp simd safelen (8)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+int d[1024];
+
+int
+test_simduid1 (int j, int b)
+{
+  int l, c = 0;
+#pragma omp simd reduction(+: c)
+  for (l = 0; l < b; ++l)
+    c += d[j + l];
+  return c;
+}
+
+int
+test_simduid2 (int j, int b)
+{
+  int l, c2 = 0;
+#pragma omp simd reduction(+: c2)
+  for (l = 0; l < b; ++l)
+    c2 += d[j + l];
+  return c2;
+}
+
+/* { dg-final { scan-ipa-dump "Semantic equality hit:test_simduid1->test_simduid2" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */

Re: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[Jakub Jelinek]

On Fri, May 24, 2019 at 09:48:03AM +0200, Martin Liška wrote:
> gcc/ChangeLog:
> 
> 2019-05-23  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/90555
> 	* ipa-icf-gimple.c (func_checker::compare_loops): New function.
> 	* ipa-icf-gimple.h (func_checker::compare_loops): Likewise.
> 	* ipa-icf.c (sem_function::equals_private): Use compare_loops.
> 
> gcc/testsuite/ChangeLog:
> 
> 2019-05-23  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/90555
> 	* gcc.dg/ipa/pr90555.c: New test.

> @@ -605,6 +606,45 @@ func_checker::compare_variable_decl (tree t1, tree t2)
>    return return_with_debug (ret);
>  }
>  
> +/* Compare significant loop information.  */
> +bool
> +func_checker::compare_loops (void)
> +{
> +  struct loop *loop;
> +
> +  auto_vec <struct loop *> loops1;
> +  auto_vec <struct loop *> loops2;
> +
> +  FOR_EACH_LOOP_FN (DECL_STRUCT_FUNCTION (m_source_func_decl), loop, 0)
> +    loops1.safe_push (loop);
> +
> +  FOR_EACH_LOOP_FN (DECL_STRUCT_FUNCTION (m_target_func_decl), loop, 0)
> +    loops2.safe_push (loop);
> +
> +  gcc_assert (loops1.length () == loops2.length ());
> +
> +  for (unsigned i = 0; i < loops1.length (); i++)

I wonder how likely/unlikely it is that the loops will be ordered the same
by the iterators.  If they are the same always, then perhaps it would be
better to avoid the temporary vectors, and just do one FOR_EACH_LOOP_FN
with another manual loop_iterator use in sync with that.
If it isn't guaranteed they are ordered the same, then I think we need to
match the loops by the corresponding loop header and perhaps also verify
loop latch and other basic blocks in the loop?

> +    {
> +      struct loop *l1 = loops1[i];
> +      struct loop *l2 = loops2[i];
> +      if (l1->simdlen != l2->simdlen)
> +	return return_false_with_msg ("simdlen");
> +      if (l1->safelen != l2->safelen)
> +	return return_false_with_msg ("safelen");
> +      if (l1->can_be_parallel != l2->can_be_parallel)
> +	return return_false_with_msg ("can_be_parallel");
> +      if (l1->dont_vectorize != l2->dont_vectorize)
> +	return return_false_with_msg ("dont_vectorize");
> +      if (l1->force_vectorize != l2->force_vectorize)
> +	return return_false_with_msg ("force_vectorize");
> +      if (l1->unroll != l2->unroll)
> +	return return_false_with_msg ("unroll");
> +      if (!compare_variable_decl (l1->simduid, l2->simduid))
> +	return return_false_with_msg ("simduid");

The above looks reasonable if we are guaranteed we are talking about a loop
corresponding between the two functions.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr90555.c
> @@ -0,0 +1,67 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fopenmp-simd -O2 -mavx512f -fdump-ipa-icf" } */
> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */

Please don't use two dg-do compile directives, drop the first line,
move the third line to first one.

	Jakub

Re: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[Richard Biener]

On Fri, May 24, 2019 at 10:08 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Fri, May 24, 2019 at 09:48:03AM +0200, Martin Liška wrote:
> > gcc/ChangeLog:
> >
> > 2019-05-23  Martin Liska  <mliska@suse.cz>
> >
> >       PR ipa/90555
> >       * ipa-icf-gimple.c (func_checker::compare_loops): New function.
> >       * ipa-icf-gimple.h (func_checker::compare_loops): Likewise.
> >       * ipa-icf.c (sem_function::equals_private): Use compare_loops.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2019-05-23  Martin Liska  <mliska@suse.cz>
> >
> >       PR ipa/90555
> >       * gcc.dg/ipa/pr90555.c: New test.
>
> > @@ -605,6 +606,45 @@ func_checker::compare_variable_decl (tree t1, tree t2)
> >    return return_with_debug (ret);
> >  }
> >
> > +/* Compare significant loop information.  */
> > +bool
> > +func_checker::compare_loops (void)
> > +{
> > +  struct loop *loop;
> > +
> > +  auto_vec <struct loop *> loops1;
> > +  auto_vec <struct loop *> loops2;
> > +
> > +  FOR_EACH_LOOP_FN (DECL_STRUCT_FUNCTION (m_source_func_decl), loop, 0)
> > +    loops1.safe_push (loop);
> > +
> > +  FOR_EACH_LOOP_FN (DECL_STRUCT_FUNCTION (m_target_func_decl), loop, 0)
> > +    loops2.safe_push (loop);
> > +
> > +  gcc_assert (loops1.length () == loops2.length ());
> > +
> > +  for (unsigned i = 0; i < loops1.length (); i++)
>
> I wonder how likely/unlikely it is that the loops will be ordered the same
> by the iterators.  If they are the same always, then perhaps it would be
> better to avoid the temporary vectors, and just do one FOR_EACH_LOOP_FN
> with another manual loop_iterator use in sync with that.
> If it isn't guaranteed they are ordered the same, then I think we need to
> match the loops by the corresponding loop header and perhaps also verify
> loop latch and other basic blocks in the loop?

In fact I wouldn't even compare loops this way but compare loop
paris when I run across two equal basic-blocks that are loop
headers (bb->loop_father == bb->loop_father->header).

> > +    {
> > +      struct loop *l1 = loops1[i];
> > +      struct loop *l2 = loops2[i];
> > +      if (l1->simdlen != l2->simdlen)
> > +     return return_false_with_msg ("simdlen");
> > +      if (l1->safelen != l2->safelen)
> > +     return return_false_with_msg ("safelen");
> > +      if (l1->can_be_parallel != l2->can_be_parallel)
> > +     return return_false_with_msg ("can_be_parallel");
> > +      if (l1->dont_vectorize != l2->dont_vectorize)
> > +     return return_false_with_msg ("dont_vectorize");
> > +      if (l1->force_vectorize != l2->force_vectorize)
> > +     return return_false_with_msg ("force_vectorize");
> > +      if (l1->unroll != l2->unroll)
> > +     return return_false_with_msg ("unroll");
> > +      if (!compare_variable_decl (l1->simduid, l2->simduid))
> > +     return return_false_with_msg ("simduid");
>
> The above looks reasonable if we are guaranteed we are talking about a loop
> corresponding between the two functions.
>
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/ipa/pr90555.c
> > @@ -0,0 +1,67 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-fopenmp-simd -O2 -mavx512f -fdump-ipa-icf" } */
> > +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
>
> Please don't use two dg-do compile directives, drop the first line,
> move the third line to first one.
>
>         Jakub

Re: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[Martin Liška]

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

On 5/24/19 11:37 AM, Richard Biener wrote:
> On Fri, May 24, 2019 at 10:08 AM Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> On Fri, May 24, 2019 at 09:48:03AM +0200, Martin Liška wrote:
>>> gcc/ChangeLog:
>>>
>>> 2019-05-23  Martin Liska  <mliska@suse.cz>
>>>
>>>       PR ipa/90555
>>>       * ipa-icf-gimple.c (func_checker::compare_loops): New function.
>>>       * ipa-icf-gimple.h (func_checker::compare_loops): Likewise.
>>>       * ipa-icf.c (sem_function::equals_private): Use compare_loops.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2019-05-23  Martin Liska  <mliska@suse.cz>
>>>
>>>       PR ipa/90555
>>>       * gcc.dg/ipa/pr90555.c: New test.
>>
>>> @@ -605,6 +606,45 @@ func_checker::compare_variable_decl (tree t1, tree t2)
>>>    return return_with_debug (ret);
>>>  }
>>>
>>> +/* Compare significant loop information.  */
>>> +bool
>>> +func_checker::compare_loops (void)
>>> +{
>>> +  struct loop *loop;
>>> +
>>> +  auto_vec <struct loop *> loops1;
>>> +  auto_vec <struct loop *> loops2;
>>> +
>>> +  FOR_EACH_LOOP_FN (DECL_STRUCT_FUNCTION (m_source_func_decl), loop, 0)
>>> +    loops1.safe_push (loop);
>>> +
>>> +  FOR_EACH_LOOP_FN (DECL_STRUCT_FUNCTION (m_target_func_decl), loop, 0)
>>> +    loops2.safe_push (loop);
>>> +
>>> +  gcc_assert (loops1.length () == loops2.length ());
>>> +
>>> +  for (unsigned i = 0; i < loops1.length (); i++)
>>
>> I wonder how likely/unlikely it is that the loops will be ordered the same
>> by the iterators.  If they are the same always, then perhaps it would be
>> better to avoid the temporary vectors, and just do one FOR_EACH_LOOP_FN
>> with another manual loop_iterator use in sync with that.
>> If it isn't guaranteed they are ordered the same, then I think we need to
>> match the loops by the corresponding loop header and perhaps also verify
>> loop latch and other basic blocks in the loop?
> 
> In fact I wouldn't even compare loops this way but compare loop
> paris when I run across two equal basic-blocks that are loop
> headers (bb->loop_father == bb->loop_father->header).

That's definitely nicer.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

> 
>>> +    {
>>> +      struct loop *l1 = loops1[i];
>>> +      struct loop *l2 = loops2[i];
>>> +      if (l1->simdlen != l2->simdlen)
>>> +     return return_false_with_msg ("simdlen");
>>> +      if (l1->safelen != l2->safelen)
>>> +     return return_false_with_msg ("safelen");
>>> +      if (l1->can_be_parallel != l2->can_be_parallel)
>>> +     return return_false_with_msg ("can_be_parallel");
>>> +      if (l1->dont_vectorize != l2->dont_vectorize)
>>> +     return return_false_with_msg ("dont_vectorize");
>>> +      if (l1->force_vectorize != l2->force_vectorize)
>>> +     return return_false_with_msg ("force_vectorize");
>>> +      if (l1->unroll != l2->unroll)
>>> +     return return_false_with_msg ("unroll");
>>> +      if (!compare_variable_decl (l1->simduid, l2->simduid))
>>> +     return return_false_with_msg ("simduid");
>>
>> The above looks reasonable if we are guaranteed we are talking about a loop
>> corresponding between the two functions.
>>
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/ipa/pr90555.c
>>> @@ -0,0 +1,67 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-fopenmp-simd -O2 -mavx512f -fdump-ipa-icf" } */
>>> +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
>>
>> Please don't use two dg-do compile directives, drop the first line,
>> move the third line to first one.
>>
>>         Jakub


[-- Attachment #2: 0001-Handle-loop-fields-in-IPA-ICF-PR-ipa-90555.patch --]
[-- Type: text/x-patch, Size: 4621 bytes --]

From 306cab10e8ef918b25321132aa75d9400f7de247 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 23 May 2019 12:47:11 +0200
Subject: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

gcc/ChangeLog:

2019-05-23  Martin Liska  <mliska@suse.cz>

	PR ipa/90555
	* ipa-icf-gimple.c (func_checker::compare_loops): New function.
	* ipa-icf-gimple.h (func_checker::compare_loops): Likewise.
	(func_checker::compare_bb): Call compare_loops.

gcc/testsuite/ChangeLog:

2019-05-23  Martin Liska  <mliska@suse.cz>

	PR ipa/90555
	* gcc.dg/ipa/pr90555.c: New test.
---
 gcc/ipa-icf-gimple.c               | 31 ++++++++++++++
 gcc/ipa-icf-gimple.h               |  3 ++
 gcc/testsuite/gcc.dg/ipa/pr90555.c | 67 ++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr90555.c

diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
index 25284936bc3..92b1813ec6c 100644
--- a/gcc/ipa-icf-gimple.c
+++ b/gcc/ipa-icf-gimple.c
@@ -37,6 +37,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "ipa-utils.h"
 #include "tree-eh.h"
 #include "builtins.h"
+#include "cfgloop.h"
 
 #include "ipa-icf-gimple.h"
 
@@ -605,6 +606,33 @@ func_checker::compare_variable_decl (tree t1, tree t2)
   return return_with_debug (ret);
 }
 
+/* Compare loop information for basic blocks BB1 and BB2.  */
+
+bool
+func_checker::compare_loops (basic_block bb1, basic_block bb2)
+{
+  if ((bb1->loop_father == NULL) != (bb2->loop_father == NULL))
+    return return_false ();
+
+  struct loop *l1 = bb1->loop_father;
+  struct loop *l2 = bb2->loop_father;
+  if (l1->simdlen != l2->simdlen)
+    return return_false_with_msg ("simdlen");
+  if (l1->safelen != l2->safelen)
+    return return_false_with_msg ("safelen");
+  if (l1->can_be_parallel != l2->can_be_parallel)
+    return return_false_with_msg ("can_be_parallel");
+  if (l1->dont_vectorize != l2->dont_vectorize)
+    return return_false_with_msg ("dont_vectorize");
+  if (l1->force_vectorize != l2->force_vectorize)
+    return return_false_with_msg ("force_vectorize");
+  if (l1->unroll != l2->unroll)
+    return return_false_with_msg ("unroll");
+  if (!compare_variable_decl (l1->simduid, l2->simduid))
+    return return_false_with_msg ("simduid");
+
+  return true;
+}
 
 /* Function visits all gimple labels and creates corresponding
    mapping between basic blocks and labels.  */
@@ -727,6 +755,9 @@ func_checker::compare_bb (sem_bb *bb1, sem_bb *bb2)
   if (!gsi_end_p (gsi2))
     return return_false ();
 
+  if (!compare_loops (bb1->bb, bb2->bb))
+    return return_false ();
+
   return true;
 }
 
diff --git a/gcc/ipa-icf-gimple.h b/gcc/ipa-icf-gimple.h
index 0035db32e66..51aadced9ea 100644
--- a/gcc/ipa-icf-gimple.h
+++ b/gcc/ipa-icf-gimple.h
@@ -226,6 +226,9 @@ public:
   /* Verifies that trees T1 and T2 do correspond.  */
   bool compare_variable_decl (tree t1, tree t2);
 
+  /* Compare loop information for basic blocks BB1 and BB2.  */
+  bool compare_loops (basic_block bb1, basic_block bb2);
+
   /* Return true if types are compatible for polymorphic call analysis.
      COMPARE_PTR indicates if polymorphic type comparsion should be
      done for pointers, too.  */
diff --git a/gcc/testsuite/gcc.dg/ipa/pr90555.c b/gcc/testsuite/gcc.dg/ipa/pr90555.c
new file mode 100644
index 00000000000..a9c751cd63c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr90555.c
@@ -0,0 +1,67 @@
+/* { dg-do compile } */
+/* { dg-options "-fopenmp-simd -O2 -mavx512f -fdump-ipa-icf" } */
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+
+#define N 1024
+int a[N];
+
+void
+test_simdlen1 (void)
+{
+  int i;
+  #pragma omp simd simdlen (4)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+void
+test_simdlen2 (void)
+{
+  int i;
+  #pragma omp simd simdlen (8)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+void
+test_safelen1 (void)
+{
+  int i;
+  #pragma omp simd safelen (4)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+void
+test_safelen2 (void)
+{
+  int i;
+  #pragma omp simd safelen (8)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+int d[1024];
+
+int
+test_simduid1 (int j, int b)
+{
+  int l, c = 0;
+#pragma omp simd reduction(+: c)
+  for (l = 0; l < b; ++l)
+    c += d[j + l];
+  return c;
+}
+
+int
+test_simduid2 (int j, int b)
+{
+  int l, c2 = 0;
+#pragma omp simd reduction(+: c2)
+  for (l = 0; l < b; ++l)
+    c2 += d[j + l];
+  return c2;
+}
+
+/* { dg-final { scan-ipa-dump "Semantic equality hit:test_simduid1->test_simduid2" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
-- 
2.21.0

Re: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[Jakub Jelinek]

On Mon, May 27, 2019 at 02:18:37PM +0200, Martin Liška wrote:
> +/* Compare loop information for basic blocks BB1 and BB2.  */
> +
> +bool
> +func_checker::compare_loops (basic_block bb1, basic_block bb2)
> +{
> +  if ((bb1->loop_father == NULL) != (bb2->loop_father == NULL))
> +    return return_false ();
> +
> +  struct loop *l1 = bb1->loop_father;
> +  struct loop *l2 = bb2->loop_father;

Perhaps also
  if ((bb1 == l1->header) != (bb2 == l2->header))
    return return_false_with_msg ("header");
  if ((bb1 == l1->latch) != (bb2 == l2->latch))
    return return_false_with_msg ("latch");
too?

BTW, unrelated to this patch, what does ICF do if e.g. SSA_NAME_PTR_INFO
or SSA_NAME_RANGE_INFO is different between otherwise identical functions?
Say one having early
  if (arg > 10)
    __builtin_unreachable ();
and another one
  if (arg > - 10)
    __builtin_unreachable ();
both optimized away into SSA_NAME_RANGE_INFO of the argument before IPA (or
do we optimize that away only after IPA?).

	Jakub

Re: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[Martin Liška]

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

On 5/27/19 2:50 PM, Jakub Jelinek wrote:
> On Mon, May 27, 2019 at 02:18:37PM +0200, Martin Liška wrote:
>> +/* Compare loop information for basic blocks BB1 and BB2.  */
>> +
>> +bool
>> +func_checker::compare_loops (basic_block bb1, basic_block bb2)
>> +{
>> +  if ((bb1->loop_father == NULL) != (bb2->loop_father == NULL))
>> +    return return_false ();
>> +
>> +  struct loop *l1 = bb1->loop_father;
>> +  struct loop *l2 = bb2->loop_father;
> 
> Perhaps also
>   if ((bb1 == l1->header) != (bb2 == l2->header))
>     return return_false_with_msg ("header");
>   if ((bb1 == l1->latch) != (bb2 == l2->latch))
>     return return_false_with_msg ("latch");
> too?

Yes, makes sense.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

> 
> BTW, unrelated to this patch, what does ICF do if e.g. SSA_NAME_PTR_INFO
> or SSA_NAME_RANGE_INFO is different between otherwise identical functions?
> Say one having early
>   if (arg > 10)
>     __builtin_unreachable ();
> and another one
>   if (arg > - 10)
>     __builtin_unreachable ();
> both optimized away into SSA_NAME_RANGE_INFO of the argument before IPA (or
> do we optimize that away only after IPA?).
> 
> 	Jakub
> 

Can you please provide a self-contained test-case?

Thanks,
Martin

[-- Attachment #2: 0001-Handle-loop-fields-in-IPA-ICF-PR-ipa-90555.patch --]
[-- Type: text/x-patch, Size: 4854 bytes --]

From 82b84914c3d40094be2ff68498a9120e9b7ece0b Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 23 May 2019 12:47:11 +0200
Subject: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

gcc/ChangeLog:

2019-05-23  Martin Liska  <mliska@suse.cz>

	PR ipa/90555
	* ipa-icf-gimple.c (func_checker::compare_loops): New function.
	* ipa-icf-gimple.h (func_checker::compare_loops): Likewise.
	(func_checker::compare_bb): Call compare_loops.

gcc/testsuite/ChangeLog:

2019-05-23  Martin Liska  <mliska@suse.cz>

	PR ipa/90555
	* gcc.dg/ipa/pr90555.c: New test.
---
 gcc/ipa-icf-gimple.c               | 38 +++++++++++++++++
 gcc/ipa-icf-gimple.h               |  3 ++
 gcc/testsuite/gcc.dg/ipa/pr90555.c | 67 ++++++++++++++++++++++++++++++
 3 files changed, 108 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr90555.c

diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
index 25284936bc3..0713e125898 100644
--- a/gcc/ipa-icf-gimple.c
+++ b/gcc/ipa-icf-gimple.c
@@ -37,6 +37,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "ipa-utils.h"
 #include "tree-eh.h"
 #include "builtins.h"
+#include "cfgloop.h"
 
 #include "ipa-icf-gimple.h"
 
@@ -605,6 +606,40 @@ func_checker::compare_variable_decl (tree t1, tree t2)
   return return_with_debug (ret);
 }
 
+/* Compare loop information for basic blocks BB1 and BB2.  */
+
+bool
+func_checker::compare_loops (basic_block bb1, basic_block bb2)
+{
+  if ((bb1->loop_father == NULL) != (bb2->loop_father == NULL))
+    return return_false ();
+
+  struct loop *l1 = bb1->loop_father;
+  struct loop *l2 = bb2->loop_father;
+  if (l1 == NULL)
+    return true;
+
+  if ((bb1 == l1->header) != (bb2 == l2->header))
+    return return_false_with_msg ("header");
+  if ((bb1 == l1->latch) != (bb2 == l2->latch))
+    return return_false_with_msg ("latch");
+  if (l1->simdlen != l2->simdlen)
+    return return_false_with_msg ("simdlen");
+  if (l1->safelen != l2->safelen)
+    return return_false_with_msg ("safelen");
+  if (l1->can_be_parallel != l2->can_be_parallel)
+    return return_false_with_msg ("can_be_parallel");
+  if (l1->dont_vectorize != l2->dont_vectorize)
+    return return_false_with_msg ("dont_vectorize");
+  if (l1->force_vectorize != l2->force_vectorize)
+    return return_false_with_msg ("force_vectorize");
+  if (l1->unroll != l2->unroll)
+    return return_false_with_msg ("unroll");
+  if (!compare_variable_decl (l1->simduid, l2->simduid))
+    return return_false_with_msg ("simduid");
+
+  return true;
+}
 
 /* Function visits all gimple labels and creates corresponding
    mapping between basic blocks and labels.  */
@@ -727,6 +762,9 @@ func_checker::compare_bb (sem_bb *bb1, sem_bb *bb2)
   if (!gsi_end_p (gsi2))
     return return_false ();
 
+  if (!compare_loops (bb1->bb, bb2->bb))
+    return return_false ();
+
   return true;
 }
 
diff --git a/gcc/ipa-icf-gimple.h b/gcc/ipa-icf-gimple.h
index 0035db32e66..51aadced9ea 100644
--- a/gcc/ipa-icf-gimple.h
+++ b/gcc/ipa-icf-gimple.h
@@ -226,6 +226,9 @@ public:
   /* Verifies that trees T1 and T2 do correspond.  */
   bool compare_variable_decl (tree t1, tree t2);
 
+  /* Compare loop information for basic blocks BB1 and BB2.  */
+  bool compare_loops (basic_block bb1, basic_block bb2);
+
   /* Return true if types are compatible for polymorphic call analysis.
      COMPARE_PTR indicates if polymorphic type comparsion should be
      done for pointers, too.  */
diff --git a/gcc/testsuite/gcc.dg/ipa/pr90555.c b/gcc/testsuite/gcc.dg/ipa/pr90555.c
new file mode 100644
index 00000000000..a9c751cd63c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr90555.c
@@ -0,0 +1,67 @@
+/* { dg-do compile } */
+/* { dg-options "-fopenmp-simd -O2 -mavx512f -fdump-ipa-icf" } */
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+
+#define N 1024
+int a[N];
+
+void
+test_simdlen1 (void)
+{
+  int i;
+  #pragma omp simd simdlen (4)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+void
+test_simdlen2 (void)
+{
+  int i;
+  #pragma omp simd simdlen (8)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+void
+test_safelen1 (void)
+{
+  int i;
+  #pragma omp simd safelen (4)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+void
+test_safelen2 (void)
+{
+  int i;
+  #pragma omp simd safelen (8)
+  for (i = 0; i < N; ++i)
+    a[i] = a[i] + 1;
+}
+
+int d[1024];
+
+int
+test_simduid1 (int j, int b)
+{
+  int l, c = 0;
+#pragma omp simd reduction(+: c)
+  for (l = 0; l < b; ++l)
+    c += d[j + l];
+  return c;
+}
+
+int
+test_simduid2 (int j, int b)
+{
+  int l, c2 = 0;
+#pragma omp simd reduction(+: c2)
+  for (l = 0; l < b; ++l)
+    c2 += d[j + l];
+  return c2;
+}
+
+/* { dg-final { scan-ipa-dump "Semantic equality hit:test_simduid1->test_simduid2" "icf"  } } */
+/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
-- 
2.21.0

Re: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[Jakub Jelinek]

On Tue, May 28, 2019 at 01:29:54PM +0200, Martin Liška wrote:
> Yes, makes sense.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ok, thanks.

> > BTW, unrelated to this patch, what does ICF do if e.g. SSA_NAME_PTR_INFO
> > or SSA_NAME_RANGE_INFO is different between otherwise identical functions?
> > Say one having early
> >   if (arg > 10)
> >     __builtin_unreachable ();
> > and another one
> >   if (arg > - 10)
> >     __builtin_unreachable ();
> > both optimized away into SSA_NAME_RANGE_INFO of the argument before IPA (or
> > do we optimize that away only after IPA?).
> > 
> > 	Jakub
> > 
> 
> Can you please provide a self-contained test-case?

I don't have one so far, it might be only vrp1 that removes those and thus
be fine for the user written asserts.  E.g. with the patch for C++ ::main argc
range it could perhaps affect code generation if we decide to ICF main with
some other function with the same content (but that patch is not in).
Another thing is I'm not sure how IPA-VRP vs. ICF interact, whether first
ranges would be set for the parameters and then ICF pick one function body,
or the other way around.

	Jakub

Re: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[Martin Liška]

On 5/28/19 1:41 PM, Jakub Jelinek wrote:
> On Tue, May 28, 2019 at 01:29:54PM +0200, Martin Liška wrote:
>> Yes, makes sense.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ok, thanks.
> 
>>> BTW, unrelated to this patch, what does ICF do if e.g. SSA_NAME_PTR_INFO
>>> or SSA_NAME_RANGE_INFO is different between otherwise identical functions?
>>> Say one having early
>>>   if (arg > 10)
>>>     __builtin_unreachable ();
>>> and another one
>>>   if (arg > - 10)
>>>     __builtin_unreachable ();
>>> both optimized away into SSA_NAME_RANGE_INFO of the argument before IPA (or
>>> do we optimize that away only after IPA?).
>>>
>>> 	Jakub
>>>
>>
>> Can you please provide a self-contained test-case?
> 
> I don't have one so far, it might be only vrp1 that removes those and thus
> be fine for the user written asserts.  E.g. with the patch for C++ ::main argc
> range it could perhaps affect code generation if we decide to ICF main with
> some other function with the same content (but that patch is not in).

That's handled here:

  3493          if (DECL_NAME (source->decl)
  3494              && MAIN_NAME_P (DECL_NAME (source->decl)))
  3495            /* If merge via wrappers, picking main as the target can be
  3496               problematic.  */
  3497            source = c->members[1];


> Another thing is I'm not sure how IPA-VRP vs. ICF interact, whether first
> ranges would be set for the parameters and then ICF pick one function body,
> or the other way around.

I'll discuss that with Martin Jambor.

Martin

> 
> 	Jakub
> 

Re: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[Martin Jambor]

Hi,

On Tue, May 28 2019, Jakub Jelinek wrote:
> On Tue, May 28, 2019 at 01:29:54PM +0200, Martin Liška wrote:
>> Yes, makes sense.
>> 
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ok, thanks.
>
>> > BTW, unrelated to this patch, what does ICF do if e.g. SSA_NAME_PTR_INFO
>> > or SSA_NAME_RANGE_INFO is different between otherwise identical functions?
>> > Say one having early
>> >   if (arg > 10)
>> >     __builtin_unreachable ();
>> > and another one
>> >   if (arg > - 10)
>> >     __builtin_unreachable ();
>> > both optimized away into SSA_NAME_RANGE_INFO of the argument before IPA (or
>> > do we optimize that away only after IPA?).
>> > 
>> > 	Jakub
>> > 
>> 
>> Can you please provide a self-contained test-case?
>
> I don't have one so far, it might be only vrp1 that removes those and thus
> be fine for the user written asserts.  E.g. with the patch for C++ ::main argc
> range it could perhaps affect code generation if we decide to ICF main with
> some other function with the same content (but that patch is not in).
> Another thing is I'm not sure how IPA-VRP vs. ICF interact, whether first
> ranges would be set for the parameters and then ICF pick one function body,
> or the other way around.

IPA-ICF runs first and unifies call graph nodes.  At that point the
actual VRP information for IPA-VRP information are stored along call
graph edges (the other bit stored along nodes are types of formal
parameters), so assuming the actual call arguments have the same value
ranges in the unified bodies, it should work.

Martin

Re: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[Christophe Lyon]

Hi Martin,


On Tue, 28 May 2019 at 13:30, Martin Liška <mliska@suse.cz> wrote:
>
> On 5/27/19 2:50 PM, Jakub Jelinek wrote:
> > On Mon, May 27, 2019 at 02:18:37PM +0200, Martin Liška wrote:
> >> +/* Compare loop information for basic blocks BB1 and BB2.  */
> >> +
> >> +bool
> >> +func_checker::compare_loops (basic_block bb1, basic_block bb2)
> >> +{
> >> +  if ((bb1->loop_father == NULL) != (bb2->loop_father == NULL))
> >> +    return return_false ();
> >> +
> >> +  struct loop *l1 = bb1->loop_father;
> >> +  struct loop *l2 = bb2->loop_father;
> >
> > Perhaps also
> >   if ((bb1 == l1->header) != (bb2 == l2->header))
> >     return return_false_with_msg ("header");
> >   if ((bb1 == l1->latch) != (bb2 == l2->latch))
> >     return return_false_with_msg ("latch");
> > too?
>
> Yes, makes sense.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>

The new testcase contains:
/* { dg-do compile } */
/* { dg-options "-fopenmp-simd -O2 -mavx512f -fdump-ipa-icf" } */
/* { dg-do compile { target i?86-*-* x86_64-*-* } } */

I suspect line 3 should actually be line 1, because the test fails to
compile on non-x86 targets:
xgcc: error: unrecognized command line option '-mavx512f'

Was it your intention?

Thanks,

Christophe

> >
> > BTW, unrelated to this patch, what does ICF do if e.g. SSA_NAME_PTR_INFO
> > or SSA_NAME_RANGE_INFO is different between otherwise identical functions?
> > Say one having early
> >   if (arg > 10)
> >     __builtin_unreachable ();
> > and another one
> >   if (arg > - 10)
> >     __builtin_unreachable ();
> > both optimized away into SSA_NAME_RANGE_INFO of the argument before IPA (or
> > do we optimize that away only after IPA?).
> >
> >       Jakub
> >
>
> Can you please provide a self-contained test-case?
>
> Thanks,
> Martin

Re: [PATCH] Handle loop fields in IPA ICF (PR ipa/90555).

[Martin Liška]

On 5/28/19 10:31 PM, Christophe Lyon wrote:
> Was it your intention?

Hi.

No ;) fixed as r271729.

Thanks for reporting,
Martin