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

[Martin Jambor <mjambor@suse.cz>]

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.

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.

[...]


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

All that said, the overall approach seems correct to me.

Martin