Hi Kewen,
On Thu, May 16, 2019 at 10:35:30PM -0500, li...@linux.ibm.com wrote:
> 2) For the other part of target invalid stmt check, as the
> hook invalid_within_doloop grep data shows, no all targets
> need to check whether invalid instructions exist in doloop.
> If we scan all stmts as generic, it can waste time for those
> targets which don't need to check.
So make the default version of the hook NULL, and only run the hook if
non-null? There are many examples of this.
> Besides, the scope of
> the current check on SWITCH in rs6000 hook is wide, later
> if we want it more exact, we may need to check more stmts
> instead of single. To let target hook scan the BBs/stmts
> by itself is also more flexible.
If we'll need that flexibility, okay.
> +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))
> + {
> + gimple *stmt = gsi_stmt (gsi);
> + if (is_gimple_call (stmt) && !gimple_call_internal_p (stmt)
> + && !is_inexpensive_builtin (gimple_call_fndecl (stmt)))
> + {
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + fprintf (dump_file,
> + "predict doloop failure due to finding call.\n");
Should this really be for -all dumps only? "X failed because Y" is often
very interesting info -- and it is not much output.
(Please start the line with a capital if you end it with a period :-) )
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + fprintf (dump_file, "predict doloop failure due to"
> + "no innermost.\n");
If you paste strings (which is fine for debug output), you still need a
space between words ;-)
> +@deftypefn {Target Hook} bool TARGET_PREDICT_DOLOOP_P (struct loop
> *@var{loop})
> +Return true if we can predict it is possible to use low-overhead loops
> +for a particular loop. The parameter @var{loop} is a pointer to the loop
"... use a low-overhead loop ..."
> +which is going to be checked. This target hook is required only when the
Just remove the whole "which is going to be checked" part?
> +target supports low-overhead loops, and will help some earlier middle-end
> +passes to make some decisions.
Is it *required* when the target has doloops? And what will happen if you
do not define this hook, either if or you have doloops or if you don't?
Hook documentation often ends "The default version of this hook returns..."
which neatly answers all this.
> + /* For now, we only consider these two RTX classes, to match what we
> + get in doloop_optimize, excluding operations like zero/sign extend. */
The indentation is broken here.
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + fprintf (dump_file, "predict doloop failure due to"
> + "target specific checks.\n");
Missing space as well (and more later, please check all).
Segher
