On Wed, Jul 21, 2021 at 7:50 AM Richard Sandiford <richard.sandif...@arm.com> wrote: > > "H.J. Lu" <hjl.to...@gmail.com> writes: > > diff --git a/gcc/builtins.c b/gcc/builtins.c > > index 39ab139b7e1..1972301ce3c 100644 > > --- a/gcc/builtins.c > > +++ b/gcc/builtins.c > > @@ -3890,13 +3890,16 @@ expand_builtin_strnlen (tree exp, rtx target, > > machine_mode target_mode) > > > > static rtx > > builtin_memcpy_read_str (void *data, void *, HOST_WIDE_INT offset, > > - scalar_int_mode mode) > > + fixed_size_mode mode) > > { > > /* The REPresentation pointed to by DATA need not be a nul-terminated > > string but the caller guarantees it's large enough for MODE. */ > > const char *rep = (const char *) data; > > > > - return c_readstr (rep + offset, mode, /*nul_terminated=*/false); > > + /* NB: Vector mode in the by-pieces infrastructure is only used by > > + the memset expander. */ > > Sorry to nitpick, but I guess this might get out out-of-date. Maybe: > > /* The by-pieces infrastructure does not try to pick a vector mode > for memcpy expansion. */
Fixed. > > + return c_readstr (rep + offset, as_a <scalar_int_mode> (mode), > > + /*nul_terminated=*/false); > > } > > > > /* LEN specify length of the block of memcpy/memset operation. > > @@ -6478,14 +6481,16 @@ expand_builtin_stpncpy (tree exp, rtx) > > > > rtx > > builtin_strncpy_read_str (void *data, void *, HOST_WIDE_INT offset, > > - scalar_int_mode mode) > > + fixed_size_mode mode) > > { > > const char *str = (const char *) data; > > > > if ((unsigned HOST_WIDE_INT) offset > strlen (str)) > > return const0_rtx; > > > > - return c_readstr (str + offset, mode); > > + /* NB: Vector mode in the by-pieces infrastructure is only used by > > + the memset expander. */ > > Similarly here for strncpy expansion. Fixed. > > + return c_readstr (str + offset, as_a <scalar_int_mode> (mode)); > > } > > > > /* Helper to check the sizes of sequences and the destination of calls > > @@ -6686,30 +6691,117 @@ expand_builtin_strncpy (tree exp, rtx target) > > return NULL_RTX; > > } > > > > -/* Callback routine for store_by_pieces. Read GET_MODE_BITSIZE (MODE) > > - bytes from constant string DATA + OFFSET and return it as target > > - constant. If PREV isn't nullptr, it has the RTL info from the > > +/* Return the RTL of a register in MODE generated from PREV in the > > previous iteration. */ > > > > -rtx > > -builtin_memset_read_str (void *data, void *prevp, > > - HOST_WIDE_INT offset ATTRIBUTE_UNUSED, > > - scalar_int_mode mode) > > +static rtx > > +gen_memset_value_from_prev (by_pieces_prev *prev, fixed_size_mode mode) > > { > > - by_pieces_prev *prev = (by_pieces_prev *) prevp; > > + rtx target = nullptr; > > if (prev != nullptr && prev->data != nullptr) > > { > > /* Use the previous data in the same mode. */ > > if (prev->mode == mode) > > return prev->data; > > + > > + fixed_size_mode prev_mode = prev->mode; > > + > > + /* Don't use the previous data to write QImode if it is in a > > + vector mode. */ > > + if (VECTOR_MODE_P (prev_mode) && mode == QImode) > > + return target; > > + > > + rtx prev_rtx = prev->data; > > + > > + if (REG_P (prev_rtx) > > + && HARD_REGISTER_P (prev_rtx) > > + && lowpart_subreg_regno (REGNO (prev_rtx), prev_mode, mode) < 0) > > + { > > + /* If we can't put a hard register in MODE, first generate a > > + subreg of word mode if the previous mode is wider than word > > + mode and word mode is wider than MODE. */ > > + if (UNITS_PER_WORD < GET_MODE_SIZE (prev_mode) > > + && UNITS_PER_WORD > GET_MODE_SIZE (mode)) > > + { > > + prev_rtx = lowpart_subreg (word_mode, prev_rtx, > > + prev_mode); > > + if (prev_rtx != nullptr) > > + prev_mode = word_mode; > > + } > > + else > > + prev_rtx = nullptr; > > I don't understand this. Why not just do the: > > if (REG_P (prev_rtx) > && HARD_REGISTER_P (prev_rtx) > && lowpart_subreg_regno (REGNO (prev_rtx), prev_mode, mode) < 0) > prev_rtx = copy_to_reg (prev_rtx); > > that I suggested in the previous review? But for --- extern void *ops; void foo (int c) { __builtin_memset (ops, c, 18); } --- I got vpbroadcastb %edi, %xmm31 vmovdqa64 %xmm31, -24(%rsp) movq ops(%rip), %rax movzwl -24(%rsp), %edx vmovdqu8 %xmm31, (%rax) movw %dx, 16(%rax) ret I want to avoid store and load. I am testing if (REG_P (prev_rtx) && HARD_REGISTER_P (prev_rtx) && lowpart_subreg_regno (REGNO (prev_rtx), prev_mode, mode) < 0) { /* Find the smallest subreg mode in the same mode class which is not narrower than MODE and narrower than PREV_MODE. */ machine_mode m; fixed_size_mode candidate; FOR_EACH_MODE_IN_CLASS (m, GET_MODE_CLASS (mode)) if (is_a<fixed_size_mode> (m, &candidate)) { if (GET_MODE_SIZE (candidate) >= GET_MODE_SIZE (prev_mode)) break; if (GET_MODE_SIZE (candidate) >= GET_MODE_SIZE (mode) && lowpart_subreg_regno (REGNO (prev_rtx), prev_mode, candidate) >= 0) { target = lowpart_subreg (candidate, prev_rtx, prev_mode); prev_rtx = target; prev_mode = candidate; break; } } if (target == nullptr) prev_rtx = copy_to_reg (prev_rtx); } to get movq ops(%rip), %rax vpbroadcastb %edi, %xmm31 vmovd %xmm31, %edx movw %dx, 16(%rax) vmovdqu8 %xmm31, (%rax) ret > IMO anything that relies on a sequence of two subreg operations is > doing something wrong. > > > + } > > + if (prev_rtx != nullptr) > > + target = lowpart_subreg (mode, prev_rtx, prev_mode); > > } > > + return target; > > +} > > + > > […] > > @@ -769,21 +769,41 @@ alignment_for_piecewise_move (unsigned int > > max_pieces, unsigned int align) > > return align; > > } > > > > -/* Return the widest integer mode that is narrower than SIZE bytes. */ > > +/* Return the widest QI vector, if QI_MODE is true, or integer mode > > + that is narrower than SIZE bytes. */ > > > > -static scalar_int_mode > > -widest_int_mode_for_size (unsigned int size) > > +static fixed_size_mode > > +widest_fixed_size_mode_for_size (unsigned int size, bool qi_vector) > > { > > - scalar_int_mode result = NARROWEST_INT_MODE; > > + machine_mode result = NARROWEST_INT_MODE; > > > > gcc_checking_assert (size > 1); > > > > + /* Use QI vector only if size is wider than a WORD. */ > > + if (qi_vector && size > UNITS_PER_WORD) > > + { > > + machine_mode mode; > > + FOR_EACH_MODE_IN_CLASS (mode, MODE_VECTOR_INT) > > + if (GET_MODE_INNER (mode) == QImode > > + && GET_MODE_SIZE (mode).is_constant ()) > > + { > > + if (GET_MODE_SIZE (mode).to_constant () >= size) > > + break; > > + if (optab_handler (vec_duplicate_optab, mode) > > + != CODE_FOR_nothing) > > + result = mode; > > + } > > + > > + if (result != NARROWEST_INT_MODE) > > + return as_a <fixed_size_mode> (result); > > + } > > + > > opt_scalar_int_mode tmode; > > FOR_EACH_MODE_IN_CLASS (tmode, MODE_INT) > > if (GET_MODE_SIZE (tmode.require ()) < size) > > result = tmode.require (); > > > > - return result; > > + return as_a <fixed_size_mode> (result); > > } > > A less cast-heavy way to write this would be: > > fixed_size_mode result = NARROWEST_INT_MODE; > > gcc_checking_assert (size > 1); > > /* Use QI vector only if size is wider than a WORD. */ > if (qi_vector && size > UNITS_PER_WORD) > { > machine_mode mode; > fixed_size_mode candidate; > FOR_EACH_MODE_IN_CLASS (mode, MODE_VECTOR_INT) > if (is_a<fixed_size_mode> (mode, &candidate) > && GET_MODE_INNER (candidate) == QImode) > { > if (GET_MODE_SIZE (candidate) >= size) > break; > if (optab_handler (vec_duplicate_optab, candidate) > != CODE_FOR_nothing) > result = candidate; > } > > if (result != NARROWEST_INT_MODE) > return result; > } Fixed. > opt_scalar_int_mode tmode; > FOR_EACH_MODE_IN_CLASS (tmode, MODE_INT) > if (GET_MODE_SIZE (tmode.require ()) < size) > result = tmode.require (); > > return result; > > > @@ -1148,13 +1178,43 @@ op_by_pieces_d::get_usable_mode (scalar_int_mode > > mode, unsigned int len) > > size = GET_MODE_SIZE (mode); > > if (len >= size && prepare_mode (mode, m_align)) > > break; > > - /* NB: widest_int_mode_for_size checks SIZE > 1. */ > > - mode = widest_int_mode_for_size (size); > > + /* NB: widest_fixed_size_mode_for_size checks SIZE > 1. */ > > + mode = widest_fixed_size_mode_for_size (size, m_qi_vector_mode); > > } > > while (1); > > return mode; > > } > > > > +/* Return the smallest integer or QI vector mode that is not narrower > > + than SIZE bytes. */ > > + > > +fixed_size_mode > > +op_by_pieces_d::smallest_fixed_size_mode_for_size (unsigned int size) > > +{ > > + /* Use QI vector only for > size of WORD. */ > > + if (m_qi_vector_mode && size > UNITS_PER_WORD) > > + { > > + machine_mode mode; > > + FOR_EACH_MODE_IN_CLASS (mode, MODE_VECTOR_INT) > > + if (GET_MODE_INNER (mode) == QImode > > + && GET_MODE_SIZE (mode).is_constant ()) > > + { > > + unsigned int mode_size = GET_MODE_SIZE (mode).to_constant (); > > + > > + /* NB: Don't return a mode wider than M_LEN. */ > > + if (mode_size > m_len) > > + break; > > + > > + if (mode_size >= size > > + && (optab_handler (vec_duplicate_optab, mode) > > + != CODE_FOR_nothing)) > > + return as_a <fixed_size_mode> (mode); > > + } > > + } > > Same idea here. Fixed. > I realise this is pre-existing, but the heavy use of “NB:” seems a bit odd. > I don't think it's something that the patch needs to adopt. Noted. > Otherwise it looks good, thanks. I think the main sticking point is > the subreg thing. > > Richard > > Thanks. -- H.J.