Re: [PATCH] aarch64, builtins: Include PR registers in FUNCTION_ARG_REGNO_P etc. [PR109254]

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

Thanks for the patch and sorry for the slow reply.

Jakub Jelinek <jakub@redhat.com> writes:
> Hi!
>
> The testcase in the PR (which unfortunately because of my lack of experience
> with SVE I'm not able to turn into a runtime testcase that verifies it)
> is miscompiled on aarch64-linux in the regname pass, because while the
> function takes arguments in the p0 register, FUNCTION_ARG_REGNO_P doesn't
> reflect that, so DF doesn't know the register is used in register passing.
> It sees 2 chains with p1 register and wants to replace the second one and
> as DF doesn't know p0 is live at the start of the function, it will happily
> use p0 register even when it is used in subsequent instructions.
>
> The following patch fixes that.  FUNCTION_ARG_REGNO_P returns non-zero
> for p0-p3 (unconditionally, seems for the floating/vector registers it
> doesn't conditionalize them on TARGET_FLOAT either, but if you want,
> I can conditionalize p0-p3 on TARGET_SVE),

I agree doing it unconditionally makes sense.

> similarly targetm.calls.function_value_regno_p returns true for p0
> register if TARGET_SVE (again for consistency, that function
> conditionalizes the float/vector on TARGET_FLOAT); looking at the
> AAPCS, seems p1-p3 could be also used to return values in case of
> homogenous aggregates, but it doesn't seem GCC supports putting
> svbool_t as a member of a structure.

One testcase that uses p1-p3 for return values is:

typedef __SVBool_t fixed_bool_t __attribute__((arm_sve_vector_bits(256)));
struct s { fixed_bool_t x[4]; };
struct s f (struct s *ptr) { return *ptr; }

compiled with -msve-vector-bits=256.

> Now, that change broke bootstrap in libobjc and some
> __builtin_apply_args/__builtin_apply/__builtin_return tests.  The
> aarch64_get_reg_raw_mode hook already documents that SVE scalable arg/return
> passing is fundamentally incompatible with those builtins, but unlike
> the floating/vector regs where it forces a fixed vector mode, I think
> there is no fixed mode which could be used for p0-p3.  So, I have tweaked
> the generic code so that it uses VOIDmode return from that hook to signal
> that a register shouldn't be touched by
> __builtin_apply_args/__builtin_apply/__builtin_return
> despite being mentioned in FUNCTION_ARG_REGNO_P or
> targetm.calls.function_value_regno_p.
>
> Bootstrapped/regtested on aarch64-linux, ok for trunk?
>
> Could somebody please turn the testcase from the PR into something that
> can be included into the testsuite?

This seems to work:

/* { dg-do run { target aarch64_sve_hw } } */
/* { dg-options "-O2 -funroll-loops" } */

#include <stdio.h>
#include <arm_sve.h>

svfloat32_t __attribute__((noipa))
func_demo(svfloat32_t x, svfloat32_t y, svbool_t pg)
{
  svfloat32_t z = svadd_f32_x(pg, x, svdup_f32(0x1.800fep19f));
  svbool_t cmp = svcmplt_f32(pg, z, svdup_f32(0.0f));
  svfloat32_t zM1 = svsub_f32_x(pg, z, svdup_n_f32(1.0f));
  z = svsel_f32(cmp, zM1, z);
  svfloat32_t sum = svadd_f32_x(pg, z, y);
  return sum;
}

int
main()
{
  float arr[2];
  svfloat32_t x = svinsr_n_f32(svdup_f32(-0x1.880fep19f), 2.0f);
  svfloat32_t res = func_demo(x, svdup_f32(0.5f), svptrue_b32());
  svst1_f32(svptrue_pat_b32(SV_VL2), arr, res);
  if (arr[0] != 786561.500000 || arr[1] != -16384.500000)
    __builtin_abort ();
  return 0;
}

> 2023-03-24  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/109254
> 	* builtins.cc (apply_args_size): If targetm.calls.get_raw_arg_mode
> 	returns VOIDmode, handle it like if the register isn't used for
> 	passing arguments at all.
> 	(apply_result_size): If targetm.calls.get_raw_result_mode returns
> 	VOIDmode, handle it like if the register isn't used for returning
> 	results at all.
> 	* target.def (get_raw_result_mode, get_raw_arg_mode): Document what it
> 	means to return VOIDmode.
> 	* doc/tm.texi: Regenerated.
> 	* config/aarch64/aarch64.cc (aarch64_function_value_regno_p): Return
> 	TARGET_SVE for P0_REGNUM.
> 	(aarch64_function_arg_regno_p): Also return true for p0-p3.
> 	(aarch64_get_reg_raw_mode): Return VOIDmode for PR_REGNUM_P regs.
>
> @@ -7995,6 +7999,10 @@ aarch64_get_reg_raw_mode (int regno)
>         for SVE types are fundamentally incompatible with the
>         __builtin_return/__builtin_apply interface.  */
>      return as_a <fixed_size_mode> (V16QImode);
> +  if (PR_REGNUM_P (regno))
> +    /* For SVE PR regs, indicate that they should be ignored for
> +       __builtin_apply/__builtin_return.  */
> +    return as_a <fixed_size_mode> (VOIDmode);   

Reached me with trailing whitespace, but maybe that's a mailerism.

It's an interesting philosophical question whether VOIDmode is a
fixed-size mode, given that VOIDmode doesn't really have a size. :-)
But I'm sure in practice we already treat it like that elsewhere.
It's just the first time I remember it being so explicit.

OK for the aarch64 bits with aarch64_function_value_regno_p
changed to use the same range as aarch64_function_arg_regno_p.

Thanks,
Richard