Re: [Patch 3/4][Aarch64] v2: Implement Aarch64 SIMD ABI

[Richard Sandiford <richard.sandiford@arm.com>]

LGTM, just minor stuff.

Steve Ellcey <sellcey@cavium.com> writes:
> +/* Return true if the instruction is a call to a SIMD function, false
> +   if it is not a SIMD function or if we do not know anything about
> +   the function.  */
> +
> +static bool
> +aarch64_simd_call_p (rtx_insn *insn)
> +{
> +  rtx symbol;
> +  rtx call;
> +  tree fndecl;
> +
> +  if (!insn)
> +    return false;

Better to arrange it so that the hook never sees null insns, since there's
nothing the hook can do in that case.  The global sets should be correct
when no other information is available.

> +/* Possibly remove some registers from register set if we know they
> +   are preserved by this call, even though they are marked as not
> +   being callee saved in CALL_USED_REGISTERS.  */
> +
> +void
> +aarch64_remove_extra_call_preserved_regs (rtx_insn *insn,
> +					  HARD_REG_SET *return_set)

s/from register set/from RETURN_SET/.  But it would be better to avoid
duplicating the description of the hook so much.  Maybe:

/* Implement TARGET_REMOVE_EXTRA_CALL_PRESERVED_REGS.  If INSN calls
   a function that uses the SIMD ABI, take advantage of the extra
   call-preserved registers that the ABI provides.  */

> +{
> +  int regno;
> +
> +  if (aarch64_simd_call_p (insn))
> +    {
> +      for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
> +	if (FP_SIMD_SAVED_REGNUM_P (regno))
> +	  CLEAR_HARD_REG_BIT (*return_set, regno);
> +    }

Might as well use:

    for (int regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)

> diff --git a/gcc/target.def b/gcc/target.def
> index 4b166d1..25be927 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -5757,6 +5757,12 @@ for targets that don't have partly call-clobbered registers.",
>   bool, (unsigned int regno, machine_mode mode),
>   hook_bool_uint_mode_false)
>  
> +DEFHOOK
> +(remove_extra_call_preserved_regs,
> + "This hook removes some registers from the callee used register set.",

Think a bit more detail would be useful here.  E.g.:

 "This hook removes registers from the set of call-clobbered registers\n\
in @var{used_regs} if, contrary to the default rules, something guarantees\n\
that @samp{insn} preserves those registers.  For example, some targets\n\
support variant ABIs in which functions preserve more registers than\n\
normal functions would.  Removing those extra registers from @var{used_regs}\n\
can lead to better register allocation.\n\
\n\
The default implementation does nothing, which is always safe.\n\
Defining the hook is purely an optimization."

> + void, (rtx_insn *insn, HARD_REG_SET *used_regs),
> + default_remove_extra_call_preserved_regs)

You need to declare this in targhooks.h.  Please sanity-test on
something like x86 that doesn't override the hook.

> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> index 3d8b3b9..a9fb101 100644
> --- a/gcc/targhooks.c
> +++ b/gcc/targhooks.c
> @@ -2372,4 +2372,11 @@ default_speculation_safe_value (machine_mode mode ATTRIBUTE_UNUSED,
>    return result;
>  }
>  
> +void
> +default_remove_extra_call_preserved_regs (rtx_insn *insn ATTRIBUTE_UNUSED,
> +					  HARD_REG_SET *used_regs
> +						ATTRIBUTE_UNUSED)
> +{
> +}

Seems easier to leave out the parameter names and drop the ATTRIBUTE_UNUSED.
The formatting wouldn't be as awkward that way.

Thanks,
Richard