On Mon, Jun 28, 2021 at 5:36 AM Richard Sandiford <richard.sandif...@arm.com> wrote: > > "H.J. Lu" <hjl.to...@gmail.com> writes: > > On Sun, Jun 27, 2021 at 2:00 PM Richard Sandiford > > <richard.sandif...@arm.com> wrote: > >> > >> "H.J. Lu via Gcc-patches" <gcc-patches@gcc.gnu.org> writes: > >> > On Sun, Jun 27, 2021 at 1:43 AM Richard Sandiford > >> > <richard.sandif...@arm.com> wrote: > >> >> > >> >> "H.J. Lu" <hjl.to...@gmail.com> writes: > >> >> > 1. Update vec_duplicate to allow to fail so that backend can only > >> >> > allow > >> >> > broadcasting an integer constant to a vector when broadcast > >> >> > instruction > >> >> > is available. This can be used by memset expander to avoid > >> >> > vec_duplicate > >> >> > when loading from constant pool is more efficient. > >> >> > >> >> I don't see any changes in target-independent code though, other than > >> >> the doc update. It's still the case that (existing) uses of > >> >> vec_duplicate_optab do not allow it to fail. > >> > > >> > I have a followup patch set on > >> > > >> > https://gitlab.com/x86-gcc/gcc/-/commits/users/hjl/pieces/broadcast > >> > > >> > to use it to expand memset with vector broadcast: > >> > > >> > https://gitlab.com/x86-gcc/gcc/-/commit/991c87f8a83ca736ae9ed92baa3ebadca289f6e3 > >> > > >> > For SSE2 which doesn't have vector broadcast, the constant vector > >> > broadcast > >> > expander returns FAIL and load from constant pool will be used. > >> > >> Hmm, but as Jeff and I mentioned in the earlier replies, > >> vec_duplicate_optab shouldn't be used for constants. Constants > >> should go via the move expanders instead. > >> > >> In a previous message I suggested: > >> > >> … would it work to change: > >> > >> /* Try using vec_duplicate_optab for uniform vectors. */ > >> if (!TREE_SIDE_EFFECTS (exp) > >> && VECTOR_MODE_P (mode) > >> && eltmode == GET_MODE_INNER (mode) > >> && ((icode = optab_handler (vec_duplicate_optab, mode)) > >> != CODE_FOR_nothing) > >> && (elt = uniform_vector_p (exp))) > >> > >> to something like: > >> > >> /* Try using vec_duplicate_optab for uniform vectors. */ > >> if (!TREE_SIDE_EFFECTS (exp) > >> && VECTOR_MODE_P (mode) > >> && eltmode == GET_MODE_INNER (mode) > >> && (elt = uniform_vector_p (exp))) > >> { > >> if (TREE_CODE (elt) == INTEGER_CST > >> || TREE_CODE (elt) == POLY_INT_CST > >> || TREE_CODE (elt) == REAL_CST > >> || TREE_CODE (elt) == FIXED_CST) > >> { > >> rtx src = gen_const_vec_duplicate (mode, expand_normal > >> (node)); > >> emit_move_insn (target, src); > >> break; > >> } > >> … > >> } > >> > >> if that code was the source of the constant operand. If we're adding a > >> new use of vec_duplicate_optab then that should be similarly protected > >> against constant operands. > >> > > > > Your comments apply to my initial vec_duplicate patch that caused the > > gcc.dg/pr100239.c failure. It has been fixed by > > > > commit ffe3a37f54ab866d85bdde48c2a32be5e09d8515 > > Author: Richard Biener <rguent...@suse.de> > > Date: Mon Jun 7 20:08:13 2021 +0200 > > > > middle-end/100951 - make sure to generate VECTOR_CST in lowering > > > > When vector lowering creates piecewise ops make sure to create > > VECTOR_CSTs instead of CONSTRUCTORs when possible. > > > > The problem I am running into now is in my memset vector broadcast > > patch. In order to optimize vector broadcast for memset, I need to > > generate a pseudo register for > > > > __builtin_memset (ops, 3, 38); > > > > only when vector broadcast is available: > > > > rtx target = nullptr; > > > > unsigned int nunits = GET_MODE_SIZE (mode) / GET_MODE_SIZE (QImode); > > machine_mode vector_mode; > > if (!mode_for_vector (QImode, nunits).exists (&vector_mode)) > > gcc_unreachable (); > > > > enum insn_code icode = optab_handler (vec_duplicate_optab, > > vector_mode); > > if (icode != CODE_FOR_nothing) > > { > > rtx reg = targetm.gen_memset_scratch_rtx (vector_mode); > > class expand_operand ops[2]; > > create_output_operand (&ops[0], reg, vector_mode); > > create_input_operand (&ops[1], data, QImode); > > if (maybe_expand_insn (icode, 2, ops)) > > { > > if (!rtx_equal_p (reg, ops[0].value)) > > emit_move_insn (reg, ops[0].value); > > target = lowpart_subreg (mode, reg, vector_mode); > > } > > } > > > > return target; <<< Return nullptr to load from constant pool. > > I don't think this is a correct use of vec_duplicate_optab. If the > scalar operand is a constant then the move should always go through > the move expanders instead, as a move from a CONST_VECTOR.
Like this? enum insn_code icode = optab_handler (vec_duplicate_optab, vector_mode); if (icode != CODE_FOR_nothing) { rtx reg = targetm.gen_memset_scratch_rtx (vector_mode); if (CONST_INT_P (data)) { /* Use the move expander with CONST_VECTOR. */ rtvec v = rtvec_alloc (nunits); for (unsigned int i = 0; i < nunits; i++) RTVEC_ELT (v, i) = data; rtx const_vec = gen_rtx_CONST_VECTOR (vector_mode, v); emit_move_insn (reg, const_vec); } else { class expand_operand ops[2]; create_output_operand (&ops[0], reg, vector_mode); create_input_operand (&ops[1], data, QImode); expand_insn (icode, 2, ops); if (!rtx_equal_p (reg, ops[0].value)) emit_move_insn (reg, ops[0].value); } target = lowpart_subreg (mode, reg, vector_mode); } -- H.J.