James Greenhalgh <james.greenha...@arm.com> writes: > On Fri, Jan 05, 2018 at 11:26:59AM +0000, Richard Sandiford wrote: >> Ping. Here's the patch updated to apply on top of the v8.4 and >> __builtin_load_no_speculate support. >> >> Richard Sandiford <richard.sandif...@linaro.org> writes: >> > This patch switches the AArch64 port to use 2 poly_int coefficients >> > and updates code as necessary to keep it compiling. >> > >> > One potentially-significant change is to >> > aarch64_hard_regno_caller_save_mode. The old implementation >> > was written in a pretty conservative way: it changed the default >> > behaviour for single-register values, but used the default handling >> > for multi-register values. >> > >> > I don't think that's necessary, since the interesting cases for this >> > macro are usually the single-register ones. Multi-register modes take >> > up the whole of the constituent registers and the move patterns for all >> > multi-register modes should be equally good. >> > >> > Using the original mode for multi-register cases stops us from using >> > SVE modes to spill multi-register NEON values. This was caught by >> > gcc.c-torture/execute/pr47538.c. >> > >> > Also, aarch64_shift_truncation_mask used GET_MODE_BITSIZE - 1. >> > GET_MODE_UNIT_BITSIZE - 1 is equivalent for the cases that it handles >> > (which are all scalars), and I think it's more obvious, since if we ever >> > do use this for elementwise shifts of vector modes, the mask will depend >> > on the number of bits in each element rather than the number of bits in >> > the whole vector. > > This is OK for trunk, with whatever modifications are needed to make the > rebase work. I do have one question and one minor comment; you can assume > that if you do make modifications in response to these that the patch is > still OK.
Thanks! >> Index: gcc/config/aarch64/aarch64.c >> =================================================================== >> --- gcc/config/aarch64/aarch64.c 2018-01-05 11:24:44.647408566 +0000 >> +++ gcc/config/aarch64/aarch64.c 2018-01-05 11:24:44.867399574 +0000 >> @@ -2262,8 +2258,12 @@ aarch64_pass_by_reference (cumulative_ar >> int nregs; >> >> /* GET_MODE_SIZE (BLKmode) is useless since it is 0. */ >> - size = (mode == BLKmode && type) >> - ? int_size_in_bytes (type) : (int) GET_MODE_SIZE (mode); >> + if (mode == BLKmode && type) >> + size = int_size_in_bytes (type); >> + else >> + /* No frontends can create types with variable-sized modes, so we >> + shouldn't be asked to pass or return them. */ >> + size = GET_MODE_SIZE (mode).to_constant (); > > I presume that the assertion in your comment is checked in the > GET_MODE_SIZE (mode).to_constant (); call? If not, is it worth making the > assert explicit here? Yeah, to_constant is an asserting operation. Maybe my history with bad naming decisions is cropping up again here though. Perhaps it would be clearer as "force_constant" or "require_constant" instead? >> @@ -11874,8 +11921,9 @@ aarch64_simd_valid_immediate (rtx op, si >> unsigned int n_elts; >> if (const_vec_duplicate_p (op, &elt)) >> n_elts = 1; >> - else if (GET_CODE (op) == CONST_VECTOR) >> - n_elts = CONST_VECTOR_NUNITS (op); >> + else if (GET_CODE (op) == CONST_VECTOR >> + && CONST_VECTOR_NUNITS (op).is_constant (&n_elts)) >> + ; >> else >> return false; >> > > A comment in the empty else if case would be useful for clarity. OK, I'll go with: /* N_ELTS set above. */; Thanks, Richard
