Re: [PATCH v4 2/3] Add predict_doloop_p target hook

[Segher Boessenkool <segher at kernel dot crashing dot org>]

Hi Kewen,

On Thu, Jun 13, 2019 at 01:50:05PM +0800, Kewen.Lin wrote:
> Comparing with the previous version, I dropped the checks
> on costly niter and invalid stmt scanning.  As Richard's
> suggestion, keep this as simple as possible, refine it 
> if finding any cases which matter later.

I think we'll want the invalid statement thing back pretty soon, it just
happens too often (20% of the time), and it makes ivopts make bad decisions.
It does not matter (much) for the performance of the generated code, but
still :-)

> +/* Predict whether the given loop in gimple will be transformed in the RTL
> +   doloop_optimize pass.  This is for rs6000 target specific.  */

Everything in rs6000/ is target-specific ;-)  Remove that part?

> +  /* On rs6000, targetm.can_use_doloop_p is actually
> +     can_use_doloop_if_innermost.  Just ensure it's innermost.  */

"the loop is innermost".

> +	fprintf (dump_file, "Predict doloop failure due to"
> +			    " no innermost.\n");

"due to no innermost" isn't great, but I don't know how th phrase it
better, either :-/  It's just debug output of course, so not so very
important.

> +This target hook is required only when the target supports low-overhead
> +loops, and will help some earlier middle-end passes to make some decisions.

Maybe just say it is for ivopts?  Or say that targets that do support
low-overhead loops *should* implement it?

> +/* True if we can predict this loop is possible to be transformed to a
> +   low-overhead loop, otherwise returns false.

"Return true if we predict the loop LOOP will be transformed to a
low-overhead loop, otherwise return false"?  Or mention doloop_optimize
like below:

> +/* Predict whether the given loop will be transformed in the RTL
> +   doloop_optimize pass.  Attempt to duplicate some doloop_optimize checks.
> +   This is only for target independent checks, see targetm.predict_doloop_p
> +   for the target dependent ones.
> +
> +   Note that according to some initial investigation, some checks like costly
> +   niter check and invalid stmt scanning don't have much gains among general
> +   cases, so keep this as simple as possible first.
> +
> +   Some RTL specific checks seems unable to be checked in gimple, if any new
> +   checks or easy checks _are_ missing here, please add them.  */

Good useful note, thanks :-)

The rs6000 part is okay for trunk.  Thanks!


Segher