Re: [PATCH 15/40] graphite: Extend SCoP detection dump output

[Richard Biener <rguenther@suse.de>]

On Wed, 18 May 2022, Harwath, Frederik wrote:

> Hi Richard,
> 
> On Tue, 2022-05-17 at 08:21 +0000, Richard Biener wrote:
> > On Mon, 16 May 2022, Tobias Burnus wrote:
> >
> > > As requested by Richard: Rediffed patch.
> > >
> > > Changes: s/.c/.cc/ + some whitespace changes.
> > > (At least in my email reader, some <tab> were lost. I also fixed
> > > too-long line
> > > issues.)
> > >
> > > In addition, FOR_EACH_LOOP was replaced by 'for (auto loop : ...'
> > > (macro was removed late in GCC 12 development ? r12-2605-
> > > ge41ba804ba5f5c)
> > >
> > > Otherwise, it should be identical to Frederik's patch, earlier in
> > > this thread.
> > >
> > > On 15.12.21 16:54, Frederik Harwath wrote:
> > > > Extend dump output to make understanding why Graphite rejects to
> > > > include a loop in a SCoP easier (for GCC developers).
> > >
> > > OK for mainline?
> >
> > +  if (printed)
> > +    fprintf (file, "\b\b");
> >
> > please find other means of omitting ", ", like by printing it
> > _before_ the number but only for the second and following loop
> > number.
> 
> Done.
> 
> >
> > I'll also note that
> >
> > +static void
> > +print_sese_loop_numbers (FILE *file, sese_l sese)
> > +{
> > +  bool printed = false;
> > +  for (auto loop : loops_list (cfun, 0))
> > +    {
> > +      if (loop_in_sese_p (loop, sese))
> > +       fprintf (file, "%d, ", loop->num);
> > +      printed = true;
> > +    }
> >
> > is hardly optimal.  Please instead iterate over
> > sese.entry->dest->loop_father and children instead which you can do
> > by passing that as extra argument to loops_list.
> 
> Done.
> 
> This had to be extended a little bit, because a SCoP
> can consist of consecutive loop-nests and iterating
> only over "loops_list (cfun, LI_INCLUDE_ROOT, sese.entry->dest-
> >loop_father))" would output only the loops from the first
> loop-nest in the SCoP (cf. the test file scop-22a.c that I added).
> 
> >
> > +
> > +  if (dump_file && dump_flags & TDF_DETAILS)
> > +    {
> > +      fprintf (dump_file, "Loops in SCoP: ");
> > +      for (auto loop : loops_list (cfun, 0))
> > +       if (loop_in_sese_p (loop, s))
> > +         fprintf (dump_file, "%d ", loop->num);
> > +      fprintf (dump_file, "\n");
> > +    }
> >
> > you are duplicating functionality of the function you just added ...
> >
> 
> Fixed.
> 
> > Otherwise looks OK to me.
> 
> Can I commit the revised patch?

Yes.

Thanks,
Richard.