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

[Martin Jambor <mjambor@suse.cz>]

Hi,

On Thu, Sep 16 2021, Kewen.Lin wrote:
> Hi Martin,
>
> Thanks for the review comments!
>
> on 2021/9/15 下午8:51, Martin Jambor wrote:
>> Hi,
>> 
>> since this is inlining-related, I would somewhat prefer Honza to have a
>> look too, but I have the following comments:
>> 
>> On Wed, Sep 08 2021, Kewen.Lin wrote:
>>>
>> 
>> [...]
>> 
>>> diff --git a/gcc/ipa-fnsummary.h b/gcc/ipa-fnsummary.h
>>> index 78399b0b9bb..300b8da4507 100644
>>> --- a/gcc/ipa-fnsummary.h
>>> +++ b/gcc/ipa-fnsummary.h
>>> @@ -193,6 +194,9 @@ public:
>>>    vec<ipa_freqcounting_predicate, va_gc> *loop_strides;
>>>    /* Parameters tested by builtin_constant_p.  */
>>>    vec<int, va_heap, vl_ptr> GTY((skip)) builtin_constant_p_parms;
>>> +  /* Like fp_expressions, but it's to hold some target specific information,
>>> +     such as some target specific isa flags.  */
>>> +  auto_vec<HOST_WIDE_INT> GTY((skip)) target_info;
>>>    /* Estimated growth for inlining all copies of the function before start
>>>       of small functions inlining.
>>>       This value will get out of date as the callers are duplicated, but
>> 
>> Segher already wrote in the first thread that a vector of HOST_WIDE_INTs
>> is an overkill and I agree.  So at least make the new field just a
>> HOST_WIDE_INT or better yet, an unsigned int.  But I would even go
>> further and make target_info only a 16-bit bit-field, place it after the
>> other bit-fields in class ipa_fn_summary and pass it to the hooks as
>> uint16_t.  Unless you have plans which require more space, I think we
>> should be conservative here.
>> 
>
> OK, yeah, the consideration is mainly for the scenario that target has
> a few bits to care about.  I just realized that to avoid inefficient
> bitwise operation for mapping target info bits to isa_flag bits, target
> can rearrange the sparse bits in isa_flag, so it's not a deal.
> Thanks for re-raising this!  I'll use the 16 bits bit-field in v3 as you
> suggested, if you don't mind, I will put it before the existing bit-fields
> to have a good alignment.

All right.

>
>> I am also not sure if I agree that the field should not be streamed for
>> offloading, but since we do not have an offloading compiler needing them
>> I guess for now that is OK. But it should be documented in the comment
>> describing the field that it is not streamed to offloading compilers.
>> 
>
> Good point, will add it in v3.
>
>> [...]
>> 
>> 
>>> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
>>> index 2470937460f..72091b6193f 100644
>>> --- a/gcc/ipa-fnsummary.c
>>> +++ b/gcc/ipa-fnsummary.c
>>> @@ -2608,6 +2617,7 @@ analyze_function_body (struct cgraph_node *node, bool early)
>>>    info->conds = NULL;
>>>    info->size_time_table.release ();
>>>    info->call_size_time_table.release ();
>>> +  info->target_info.release();
>>>  
>>>    /* When optimizing and analyzing for IPA inliner, initialize loop optimizer
>>>       so we can produce proper inline hints.
>>> @@ -2659,6 +2669,12 @@ analyze_function_body (struct cgraph_node *node, bool early)
>>>  			   bb_predicate,
>>>  		           bb_predicate);
>>>  
>>> +  /* Only look for target information for inlinable functions.  */
>>> +  bool scan_for_target_info =
>>> +    info->inlinable
>>> +    && targetm.target_option.need_ipa_fn_target_info (node->decl,
>>> +						      info->target_info);
>>> +
>>>    if (fbi.info)
>>>      compute_bb_predicates (&fbi, node, info, params_summary);
>>>    const profile_count entry_count = ENTRY_BLOCK_PTR_FOR_FN (cfun)->count;
>>> @@ -2876,6 +2892,10 @@ analyze_function_body (struct cgraph_node *node, bool early)
>>>  		  if (dump_file)
>>>  		    fprintf (dump_file, "   fp_expression set\n");
>>>  		}
>>> +	      if (scan_for_target_info)
>>> +		scan_for_target_info =
>>> +		  targetm.target_option.update_ipa_fn_target_info
>>> +		  (info->target_info, stmt);
>>>  	    }
>> 
>> Practically it probably does not matter, but why is this in the "if
>> (this_time || this_size)" block?  Although I can see that setting
>> fp_expression is also done that way... but it seems like copying a
>> mistake to me.
>
> Yeah, I felt target info scanning is similar to fp_expression scanning,
> so I just followed the same way.  If I read it right, the case
> !(this_time || this_size) means the STMT won't be weighted to any RTL
> insn from both time and size perspectives, so guarding it seems to avoid
> unnecessary scannings.  I assumed that target bifs and inline asm would
> not be evaluated as zero cost, it seems safe so far for HTM usage.
>
> Do you worry about some special STMT which is weighted to zero but it's
> necessarily to be checked for target info in a long term?
> If so, I'll move it out in v3.

It seems that gimple_call_internal_p statements are always costed to
zero and I am wondering whether those are something that targets would
want to look out for in the future.

But hopefully anyone implementing that in the future would come up with
a testcase and would need to fix this to have the testcase pass.

Thanks,

Martin

>> 
>> All that said, the overall approach seems correct to me.
>> 
>
> Thanks again.
> BR,
> Kewen