Hi Segher,
Thanks for the comments!
on 2022/2/7 下午3:53, Segher Boessenkool wrote:
> Hi!
>
> On Wed, Jan 05, 2022 at 03:34:41PM +0800, Kewen.Lin wrote:
>> This patch is to fix the inconsistent behaviors for non-LTO mode
>> and LTO mode. As Martin pointed out, currently the function
>> rs6000_can_inline_p simply makes it inlinable if callee_tree is
>> NULL, but it's unexpected, we should use the command line options
>> from target_option_default_node as default.
>
>> --- a/gcc/config/rs6000/rs6000.c
>> +++ b/gcc/config/rs6000/rs6000.c
>> @@ -25379,55 +25379,87 @@ rs6000_update_ipa_fn_target_info (unsigned int
>> &info, const gimple *stmt)
>> static bool
>> rs6000_can_inline_p (tree caller, tree callee)
>> {
>> - bool ret = false;
>> tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller);
>> tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee);
>>
>> - /* If the callee has no option attributes, then it is ok to inline. */
>> + /* If the caller/callee has option attributes, then use them.
>> + Otherwise, use the command line options. */
>> if (!callee_tree)
>> - ret = true;
>> -
>> - else
>> - {
>> - HOST_WIDE_INT caller_isa;
>> - struct cl_target_option *callee_opts = TREE_TARGET_OPTION
>> (callee_tree);
>> - HOST_WIDE_INT callee_isa = callee_opts->x_rs6000_isa_flags;
>> - HOST_WIDE_INT explicit_isa = callee_opts->x_rs6000_isa_flags_explicit;
>> + callee_tree = target_option_default_node;
>> + if (!caller_tree)
>> + caller_tree = target_option_default_node;
>> +
>> + struct cl_target_option *caller_opts = TREE_TARGET_OPTION (caller_tree);
>> + struct cl_target_option *callee_opts = TREE_TARGET_OPTION (callee_tree);
>> + HOST_WIDE_INT caller_isa = caller_opts->x_rs6000_isa_flags;
>> + HOST_WIDE_INT callee_isa = callee_opts->x_rs6000_isa_flags;
>> +
>> + bool always_inline
>> + = DECL_DISREGARD_INLINE_LIMITS (callee)
>> + && lookup_attribute ("always_inline", DECL_ATTRIBUTES (callee));
>> +
>> + /* Some features can be tolerated for always inlines. */
>> + unsigned HOST_WIDE_INT always_inline_safe_mask
>> + /* Fusion option masks. */
>> + = OPTION_MASK_P8_FUSION | OPTION_MASK_P10_FUSION
>> + | OPTION_MASK_P8_FUSION_SIGN | OPTION_MASK_P10_FUSION
>> + | OPTION_MASK_P10_FUSION_LD_CMPI | OPTION_MASK_P10_FUSION_2LOGICAL
>> + | OPTION_MASK_P10_FUSION_LOGADD | OPTION_MASK_P10_FUSION_ADDLOG
>> + | OPTION_MASK_P10_FUSION_2ADD
>> + /* Like fusion, some option masks which are just for optimization. */
>> + | OPTION_MASK_SAVE_TOC_INDIRECT | OPTION_MASK_PCREL_OPT;
>
> Why is this? I would expect only two or three options to be *not* safe!
>
> You have a strange mix of internal options (the FUSION* things) and some
> other options that do not change the semantics at all. But this is true
> for almost *all* options we have! What would not allow a callee that is
> allowed to use some newer ISA's insns into a caller that does not allow
> them? Both when it ends up using those insns and when it does not, the
> end result is valid. If there is something internal to GCC that causes
> ICEs we need to do something about *that*!
>
The reason why I used these options is that all of them are mainly for
performance purpose, they are not to control if some insns are available
to generate. Sorry that if they look strangely mixed.
IMHO it's not "true for almost *all* options we have". For the
options at the beginning of rs6000 options manual page.
-mpowerpc-gpopt [guarding bifs]
-mpowerpc-gfxopt [guarding bifs]
-mpowerpc64
-mmfcrf
-mpopcntb [guarding bifs]
-mpopcntd [guarding bifs]
-mfprnd
-mcmpb [guarding bifs]
-mhard-dfp [guarding bifs]
...
Imagining that if the callee has some built-in functions or some inline
assembly which requires the related hardware feature provided, I think we
shouldn't claim they are safe even with always_inline. For example, for
built-in functions, there will be some error messages without related
features supported.
>> + /* Some features are totally safe for inlining (or always inlines),
>> + let's exclude them from the following checkings. */
>> + HOST_WIDE_INT safe_mask = always_inline ? always_inline_safe_mask : 0;
>> + cgraph_node *callee_node = cgraph_node::get (callee);
>> + if (ipa_fn_summaries && ipa_fn_summaries->get (callee_node) != NULL)
>> + {
>> + unsigned int info = ipa_fn_summaries->get (callee_node)->target_info;
>> + if ((info & RS6000_FN_TARGET_INFO_HTM) == 0)
>> + safe_mask |= OPTION_MASK_HTM;
>> + }
>
> always_inline means *always*. If the compiler cannot do this it should
> not, but then error; in all other cases it should do it.
>
Maybe there are some user cases we want to tolerate? Like this PR?
> This patch is pretty much equal to not allowing any inlining if the
> caller and callee have not identical compilation options. So sure, it
> causes us to not have any unwanted inlining, but it does that by
> preventing all other inlining at the same time.
>
hmm, no, IMHO the current implementation is more strict and does what you
described here, this patch is to try to relax it a bit for always_inline
inlining, it makes us not count some flags into mismatch checking.
If you are talking about the code:
>> if (!callee_tree)
>> - ret = true;
This is one existing bug which causes the inconsistence between LTO and
non-LTO mode.
BR,
Kewen
