Thanks for the patch and sorry for the slow reply.
Jakub Jelinek <ja...@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 <ja...@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
