Hi Segher,
on 2019/5/17 下午2:49, Segher Boessenkool wrote:
> 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.
>
Good tips!
>> 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.
>
OK, I thought users can firstly check the predict_doloop result whether
meet theirs' expectation and use -details dump for further check why.
And IVOPTs also uses "TDF_DETAILS" for dumping much.
Just remove the flags check? or which TDF level you suggested?
> (Please start the line with a capital if you end it with a period :-) )
>
Some contexts seems missing for this? Or it's for dumping string?
>> + 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 ;-)
>
Good catches here and later! :)
>> +@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.
>
OK, will update it with "the default version of this hook return false."
>> + /* 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.
>
Good catch, contrib/check_GNU_style.sh did NOT catch this. :(
Thanks,
Kewen
>> + 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
>
