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

  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), 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.

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?

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.


	Jakub
  

Hi!

On Fri, Mar 24, 2023 at 10:59:36PM +0100, Jakub Jelinek via Gcc-patches wrote:
> 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.

I'd like to ping this patch.
https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614594.html

Thanks

	Jakub
  

On 3/24/23 15:59, Jakub Jelinek via Gcc-patches wrote:
> 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), 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.
> 
> 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?
> 
> 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.
Generic bits are OK by me.  The aarch64 bits looks sensible, but I'd 
like to give the aarch folks one more chance to chime in.

So OK for the trunk Monday if you haven't heard otherwise.

jeff
  

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