[PATCH v2 2/3] Add predict_doloop_p target hook

[Richard Biener]

On Wed, 15 May 2019, Segher Boessenkool wrote:

> On Wed, May 15, 2019 at 10:40:09AM +0800, Bin.Cheng wrote:
> > I wonder if you can factor out generic part into GIMPLE and leave
> > target hook as specific as possible?
> 
> Less GIMPLE handling code in the backend would probably be good, yes.  Less
> of the mechanics at least.
> 
> > > +static bool
> > > +invalid_insn_for_doloop_p (struct loop *loop)
> > > +{
> > > +  basic_block *body = get_loop_body (loop);
> > > +  gimple_stmt_iterator gsi;
> > > +
> > > +  for (unsigned i = 0; i < loop->num_nodes; i++)
> > > +    for (gsi = gsi_start_bb (body[i]); !gsi_end_p (gsi); gsi_next (&gsi))
> > The loops on bbs/stmts seem general and can be put in GIMPLE.  So a
> > target hook taking gimple_stmt parameter and returning if the stmt
> > blocks doloop is enough?
> 
> I can't think of any arch where that will not work, for most things.  The
> other big "can we make this loop a doloop" factor is the loop (nest) itself:
> on some archs there can be more than one doloop at once.  Maybe GCC doesn't
> target any such target?
> 
> > > +  /* Similar to doloop_optimize, check whether iteration count too small
> > > +     and not profitable.  */
> > > +  HOST_WIDE_INT est_niter = get_estimated_loop_iterations_int (loop);
> > > +  if (est_niter == -1)
> > > +    est_niter = get_likely_max_loop_iterations_int (loop);
> > > +  if (est_niter >= 0 && est_niter < 3)
> > The only probably target dependent is the magic number 3 here.  After
> > moving all generic part to ivopts, we can use a macro for the moment,
> > and improve it as a parameter if there are more doloop targets.
> 
> It is a constant 3 in current RTL code, already; making it a param would
> be welcome, indeed :-)
> 
> > > +    {
> > > +      if (dump_file && (dump_flags & TDF_DETAILS))
> > > +       fprintf (dump_file,
> > > +                "predict doloop failure due to"
> > > +                "too few iterations (%d).\n",
> > > +                (unsigned int) est_niter);
> 
> %d isn't unsigned, btw.  Just use (int), or use %u?
> 
> > Overall most of above checks can be moved out of backend, leaving only
> > more specific target hook checking on gimple_stmt?  And even that can
> > be made generic to some extend.
> 
> But will that help, will it make the code more readable / maintainable?
> 
> Most of these things aren't generic...  On Power we cannot allow function
> calls inside the loop because the count register is call-clobbered, and
> we cannot allow indirect jumps (like in many switch statements) because
> those use the count register as well.  There can of course be other reasons
> why we do not want calls or switches inside a doloop anyway, but that then
> is just a lucky coincidence ;-)

I wonder if making the doloop patterns (tried to find them in rs6000.md,
but I only see define_expands with no predicates/alternatives...)
accept any counter register, just have a preference on that special
counter reg and have the define_insn deal with RA allocating another
one by emitting a regular update & branch-on-zero?

That is, the penalty of doing that shouldn't be too big and thus
we can more optimistically cost & handle "doloops"?  I guess
the doloop.c checks are quite too strict because we have to
rely on RA being able to allocate that reg and as soon as we
need to spill it using a general reg with update & branch-on-zero
will be cheaper anyways?

Richard.