Hi!
On Wed, Sep 01, 2021 at 03:02:22PM +0800, Kewen.Lin wrote:
> It introduces two target hooks need_ipa_fn_target_info and
> update_ipa_fn_target_info. The former allows target to do
> some previous check and decides to collect target specific
> information for this function or not. For some special case,
> it can predict the analysis result and push it early without
> any scannings. The latter allows the analyze_function_body
> to pass gimple stmts down just like fp_expressions handlings,
> target can do its own tricks.
>
> To make it simple, this patch uses HOST_WIDE_INT to record the
> flags just like what we use for isa_flags. For rs6000's HTM
> need, one HOST_WIDE_INT variable is quite enough, but it seems
> good to have one auto_vec for scalability as I noticed some
> targets have more than one HOST_WIDE_INT flag. For now, this
> target information collection is only for always_inline function,
> function ipa_merge_fn_summary_after_inlining deals with target
> information merging.
These flags can in principle be separate from any flags the target
keeps, so 64 bits will be enough for a long time. If we want to
architect that better, we should really architect the way all targets
do target flags first. Let's not go there now :-)
So just one HOST_WIDE_INT, not a stack of them please?
> --- a/gcc/config/rs6000/rs6000-call.c
> +++ b/gcc/config/rs6000/rs6000-call.c
> @@ -13642,6 +13642,17 @@ rs6000_builtin_decl (unsigned code, bool
> initialize_p ATTRIBUTE_UNUSED)
> return rs6000_builtin_decls[code];
> }
>
> +/* Return true if the builtin with CODE has any mask bits set
> + which are specified by MASK. */
> +
> +bool
> +rs6000_builtin_mask_set_p (unsigned code, HOST_WIDE_INT mask)
> +{
> + gcc_assert (code < RS6000_BUILTIN_COUNT);
> + HOST_WIDE_INT fnmask = rs6000_builtin_info[code].mask;
> + return fnmask & mask;
> +}
The "_p" does not say that "any bits" part, which is crucial here. So
name this something like "rs6000_fn_has_any_of_these_mask_bits"? Yes
the name sucks, because this interface does :-P
Its it useful to have "any" semantics at all? Otherwise, require this
to be passed just a single bit?
The implicit "!!" (or "!= 0", same thing) that casting to bool does
might be better explicit, too? A cast to bool changes value so is more
suprising than other casts.
> + /* Assume inline asm can use any instruction features. */
> + if (gimple_code (stmt) == GIMPLE_ASM)
> + {
> + info[0] = -1;
> + return false;
> + }
What is -1 here? "All options set"? Does that work? Reliably?
> + if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_MD))
> + {
> + enum rs6000_builtins fcode =
> + (enum rs6000_builtins) DECL_MD_FUNCTION_CODE (fndecl);
> + /* HTM bifs definitely exploit HTM insns. */
> + if (rs6000_builtin_mask_set_p ((unsigned) fcode, RS6000_BTM_HTM))
Why the cast here? Please change the parameter type, instead? It is
fine to use enums specific to our backend in that backend itself :-)
> @@ -1146,6 +1147,16 @@ ipa_dump_fn_summary (FILE *f, struct cgraph_node *node)
> fprintf (f, " calls:\n");
> dump_ipa_call_summary (f, 4, node, s);
> fprintf (f, "\n");
> + HOST_WIDE_INT flags;
> + for (int i = 0; s->target_info.iterate (i, &flags); i++)
> + {
> + if (i == 0)
> + {
> + fprintf (f, " target_info flags:");
> + }
Don't use blocks around single statements please.
> + /* Only look for target information for inlinable always_inline functions.
> */
> + bool scan_for_target_info =
> + (info->inlinable
> + && DECL_DISREGARD_INLINE_LIMITS (node->decl)
> + && lookup_attribute ("always_inline", DECL_ATTRIBUTES (node->decl))
> + && targetm.target_option.need_ipa_fn_target_info (node->decl,
> + info->target_info));
Don't use unnecessary parens please.
Segher
