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

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Segher,

Thanks for the comments!

on 2021/9/3 上午1:44, Segher Boessenkool wrote:
> 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?

I considered this, it's fine to use this customized bit in the target hook,
but back to target hook can_inline_p, we have to decoded them to the bits
in isa_flags separately, it's inefficient than just using the whole mask
if the interesting bits are more.

As the discussion with Richi, theoretically speaking if target likes, it can
try to scan for many isa features with target's own desicions, there could be
much more bits.  Another thing inspiring me to make it with one vector is that
i386 port ix86_can_inline_p checks x_ix86_target_flags, x_ix86_isa_flags,
x_ix86_isa_flags2, arch and tune etc. now, one HOST_WIDE_INT seems not good
to it, if it wants to check more.  ;-)

> 
>> --- 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
> 

Thanks for the name, will fix it.  :)

> Its it useful to have "any" semantics at all?  Otherwise, require this
> to be passed just a single bit?
> 

Since we can not just pass in a bit, we have to assert it with something
like:

   gcc_assert (__builtin_popcount(mask) == 1);

to claim it's checking a single bit.  But the implementation logic still
supports checking any bits, so I thought we can just claim it to check
any bits and a single bit is just one special case.

Yeah, not sure if there is a need to check any bits, but something like
checking exists FRSQRTE and FRSQRTES bifs can pass (RS6000_BTM_FRSQRTE |
RS6000_BTM_FRSQRTES), so is it fine to keep it for any bits?

> 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.

OK, will fix it.

> 
>> +  /* 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?
> 

Good question, in the current implementation it's reliable, since we do
operation "~" first then & the interesting bits (OPTION_MASK_HTM here)
but I think you concerned some conflict bits co-exists is reasonable or
not.  I was intended to cover any future interesting bits, but I agree
it's better to just set the correpsonding intersting bits to make it clear.

Will fix it.

>> +      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 :-)
> 

Referred to the exisitng rs6000_builtin_decl, just noticed it's a hook.
Will fix it.

>> @@ -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.
> 

Good catch, will fix it.

>> +  /* 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.
> 

OK, will fix it.

BR,
Kewen.