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