Re: [PATCH] rs6000: Fix some issues in rs6000_can_inline_p [PR102059]

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

Hi!

On Wed, Sep 01, 2021 at 02:55:51PM +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 wrong, we should use the command line options
> from target_option_default_node as default.

This is not documented.

> It also replaces
> rs6000_isa_flags with the one from target_option_default_node
> when caller_tree is NULL as rs6000_isa_flags could probably
> change since initialization.

Is this enough then?  Are there no other places that use
rs6000_isa_flags?  Is it correct for us to have that variable at all
anymore?  Etc.

> +  bool always_inline =
> +    (DECL_DISREGARD_INLINE_LIMITS (callee)
> +     && lookup_attribute ("always_inline", DECL_ATTRIBUTES (callee)));

The "=" should start a line, not end it.  And please don't use
unnecessary parens.

> +  /* Some flags such as fusion can be tolerated for always inlines.  */
> +  unsigned HOST_WIDE_INT always_inline_safe_mask =
> +    (MASK_P8_FUSION | MASK_P10_FUSION | OPTION_MASK_SAVE_TOC_INDIRECT
> +     | OPTION_MASK_P8_FUSION_SIGN | 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
> +     | OPTION_MASK_PCREL_OPT);

Same.

The fusion ones are obvious.  The other two are not.  Please put those
two more obviously separate (not somewhere in the sea of fusion
options), and some comment wouldn't hurt either.  You can make it
separate statements even, make it really stand out.

Why are there OPTION_MASKs for separate P10 fusion types here, as well as
MASK_P10_FUSION?

> +
> +  if (always_inline) {
> +    caller_isa &= ~always_inline_safe_mask;
> +    callee_isa &= ~always_inline_safe_mask;
> +  }

"{" starts a new line, indented.


Segher