[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