Steve Ellcey <sell...@marvell.com> writes:
> On Wed, 2019-01-09 at 10:00 +0000, Richard Sandiford wrote:
>
> Thanks for the quick turnaround on the comments Richard. Here is a new
> version where I tried to address all the issues you raised. One thing
> I noticed is that I think your calls_have_same_clobbers_p function only
> works if, when return_call_with_max_clobbers is called with two calls
> that clobber the same set of registers, it always returns the first
> call.
>
> I don't think my original function had that guarantee but I changed it
> so that it would and documented that requirement in target.def. I
> couldn't see a better way to implement the calls_have_same_clobbers_p
> function other than doing that.
Yeah, I think that's a good guarantee to have.
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 1c300af..d88be6c 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -1655,14 +1655,56 @@ aarch64_reg_save_mode (tree fndecl, unsigned regno)
> /* Implement TARGET_HARD_REGNO_CALL_PART_CLOBBERED. The callee only saves
> the lower 64 bits of a 128-bit register. Tell the compiler the callee
> clobbers the top 64 bits when restoring the bottom 64 bits. */
>
> static bool
> -aarch64_hard_regno_call_part_clobbered (unsigned int regno, machine_mode
> mode)
> +aarch64_hard_regno_call_part_clobbered (rtx_insn *insn, unsigned int regno,
> + machine_mode mode)
> {
> - return FP_REGNUM_P (regno) && maybe_gt (GET_MODE_SIZE (mode), 8);
> + bool simd_p = insn && CALL_P (insn) && aarch64_simd_call_p (insn);
> + return FP_REGNUM_P (regno)
> + && maybe_gt (GET_MODE_SIZE (mode), simd_p ? 16 : 8);
> +}
> +
> +/* Implement TARGET_RETURN_CALL_WITH_MAX_CLOBBERS. */
> +
> +rtx_insn *
> +aarch64_return_call_with_max_clobbers (rtx_insn *call_1, rtx_insn *call_2)
> +{
> + gcc_assert (CALL_P (call_1) && CALL_P (call_2));
> +
> + if (aarch64_simd_call_p (call_1) == aarch64_simd_call_p (call_2))
> + return call_1;
> +
> + if (aarch64_simd_call_p (call_2))
> + return call_1;
> + else
> + return call_2;
Think this is simpler as:
gcc_assert (CALL_P (call_1) && CALL_P (call_2));
if (!aarch64_simd_call_p (call_1) || aarch64_simd_call_p (call_2))
return call_1;
else
return call_2;
> diff --git a/gcc/lra-lives.c b/gcc/lra-lives.c
> index a00ec38..61149e1 100644
> --- a/gcc/lra-lives.c
> +++ b/gcc/lra-lives.c
> @@ -579,22 +579,32 @@ lra_setup_reload_pseudo_preferenced_hard_reg (int regno,
> PSEUDOS_LIVE_THROUGH_CALLS and PSEUDOS_LIVE_THROUGH_SETJUMPS. */
> static inline void
> check_pseudos_live_through_calls (int regno,
> - HARD_REG_SET last_call_used_reg_set)
> + HARD_REG_SET last_call_used_reg_set,
> + rtx_insn *call_insn)
Should document the new parameter.
> @@ -906,17 +933,22 @@ process_bb_lives (basic_block bb, int &curr_point, bool
> dead_insn_p)
>
> bool flush = (! hard_reg_set_empty_p (last_call_used_reg_set)
> && ! hard_reg_set_equal_p (last_call_used_reg_set,
> - this_call_used_reg_set));
> + this_call_used_reg_set)
> + && ! calls_have_same_clobbers_p (call_insn,
> + last_call_insn));
This should be || with the current test, not &&. We need to check
that last_call_insn is nonnull first.
> EXECUTE_IF_SET_IN_SPARSESET (pseudos_live, j)
> {
> IOR_HARD_REG_SET (lra_reg_info[j].actual_call_used_reg_set,
> this_call_used_reg_set);
> +
> if (flush)
> - check_pseudos_live_through_calls
> - (j, last_call_used_reg_set);
> + check_pseudos_live_through_calls (j,
> + last_call_used_reg_set,
> + curr_insn);
> }
Should be last_call_insn rather than curr_insn. I.e. when we flush,
we apply the properties of the previous call to pseudos live after
the new call.
Looks good otherwise.
Thanks,
Richard
