Re: [RFC/PATCH] ipa-inline: Add target info into fn summary [PR102059]

[Segher Boessenkool <segher@kernel.crashing.org>]

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