On Mon, Feb 07, 2022 at 08:09:47AM +0100, Andreas Krebbel via Gcc-patches wrote:
> MASK_MVCLE is set for -Os but not for other optimization levels. In
> general it should not make much sense to inline across calls where the
> flag is different but we have to allow it for always_inline.
>
> The patch also rearranges the hook implementation a bit based on the
> recommendations from Jakub und Martin in the PR.
>
> Bootstrapped and regression tested on s390x with various arch flags.
> Will commit after giving a few days for comments.
>
> gcc/ChangeLog:
>
> PR target/104327
> * config/s390/s390.cc (s390_can_inline_p): Accept a few more flags
> if always_inline is set. Don't inline when tune differs without
> always_inline.
>
> gcc/testsuite/ChangeLog:
>
> PR target/104327
> * gcc.c-torture/compile/pr104327.c: New test.
Looks mostly good to me, except:
> ---
> gcc/config/s390/s390.cc | 66 ++++++++++++++-----
> .../gcc.c-torture/compile/pr104327.c | 15 +++++
> 2 files changed, 64 insertions(+), 17 deletions(-)
> create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr104327.c
>
> diff --git a/gcc/config/s390/s390.cc b/gcc/config/s390/s390.cc
> index 5c2a830f9f0..bbf2dd8dfb4 100644
> --- a/gcc/config/s390/s390.cc
> +++ b/gcc/config/s390/s390.cc
> @@ -16091,6 +16091,25 @@ s390_valid_target_attribute_p (tree fndecl,
> static bool
> s390_can_inline_p (tree caller, tree callee)
> {
> + unsigned HOST_WIDE_INT all_masks =
> + (MASK_64BIT | MASK_BACKCHAIN | MASK_DEBUG_ARG | MASK_ZARCH
> + | MASK_HARD_DFP | MASK_SOFT_FLOAT
> + | MASK_OPT_HTM | MASK_LONG_DOUBLE_128 | MASK_MVCLE | MASK_PACKED_STACK
> + | MASK_SMALL_EXEC | MASK_OPT_VX | MASK_ZVECTOR);
> +
> + /* Flags which if present in the callee are required in the caller as
> well. */
> + unsigned HOST_WIDE_INT caller_required_masks = MASK_OPT_HTM;
> +
> + /* Flags which affect the ABI and in general prevent inlining. */
> + unsigned HOST_WIDE_INT must_match_masks =
> + (MASK_64BIT | MASK_ZARCH | MASK_HARD_DFP | MASK_SOFT_FLOAT
> + | MASK_LONG_DOUBLE_128 | MASK_OPT_VX);
> +
> + /* Flags which we in general want to prevent inlining but accept for
> + always_inline. */
> + unsigned HOST_WIDE_INT always_inline_safe_masks =
> + MASK_MVCLE | MASK_BACKCHAIN | MASK_SMALL_EXEC;
1) formatting, = should be at the start of next line rather than end of the
line
2) all_masks, always_inline_safe_masks and caller_required_masks aren't
ever modified, perhaps make them const?
3) I wonder if there is any advantage to have all_masks with all the masks
enumerated, compared to
const HOST_WIDE_INT all_masks
= (caller_required_masks | must_match_masks | always_inline_safe_masks
| MASK_DEBUG_ARG | MASK_PACKED_STACK | MASK_ZVECTOR);
i.e. when you add a new mask, instead of listing it in all_masks
and one or more of the other vars you'd just stick it either in one
or more of those vars or in all_masks.
Jakub
