On Fri, Sep 17, 2021 at 05:42:38PM +0800, Kewen.Lin wrote:
> Against v2 [2], this v3 addressed Martin's review comments:
> - Replace HWI auto_vec with unsigned int for target_info
> to avoid overkill (also Segher's comments), adjust some
> places need to be updated for this change.
I'd have used a single HWI (always 64 bits), but an int (always at least
32 bits in GCC) is fine as well, sure.
> * config/rs6000/rs6000-internal.h
> (rs6000_fn_has_any_of_these_mask_bits): New declare.
You can break that after the ":"... Just :-)
> * doc/tm.texi.in (TARGET_UPDATE_IPA_FN_TARGET_INFO): Document new
> hook.
That easily fits one line. (Many more examples here btw).
> +bool
> +rs6000_fn_has_any_of_these_mask_bits (enum rs6000_builtins code,
> + HOST_WIDE_INT mask)
> +{
> + gcc_assert (code < RS6000_BUILTIN_COUNT);
We don't have this assert anywhere else, so lose it here as well?
If we want such checking we should make an inline accessor function for
this, and check it there. But we already do a check in
rs6000_builtin_decl (and one in def_builtin, but that one has an
off-by-one error in it).
> +extern bool rs6000_fn_has_any_of_these_mask_bits (enum rs6000_builtins code,
> + HOST_WIDE_INT mask);
The huge unwieldy name suggests it might not be the best abstraction you
could use, btw ;-)
> +static bool
> +rs6000_update_ipa_fn_target_info (unsigned int &info, const gimple *stmt)
> +{
> + /* Assume inline asm can use any instruction features. */
> + if (gimple_code (stmt) == GIMPLE_ASM)
This should be fine for HTM, but it may be a bit *too* pessimistic for
other features. We'll see when we get there :-)
> +@deftypefn {Target Hook} bool TARGET_NEED_IPA_FN_TARGET_INFO (const_tree
> @var{decl}, unsigned int& @var{info})
> +Allow target to check early whether it is necessary to analyze all gimple
> +statements in the given function to update target specific information for
> +inlining. See hook @code{update_ipa_fn_target_info} for usage example of
[ ... ]
> +The default version of this hook returns false.
And that is really the only reason to have this premature optimisation:
targets that do not care do not have to pay the price, however trivial
that price may be, which is a good idea politically ;-)
> +/* { dg-final { scan-tree-dump-times "Inlining foo/\[0-9\]* " 1 "einline"} }
> */
If you use {} instead of "" you don't need the backslashes.
> +default_update_ipa_fn_target_info (uint16_t &, const gimple *)
I'm surprised the compiler didn't warn about this btw.
The rs6000 parts are okay for trunk (with the trivial cleanups please).
Thanks!
Segher
