On Thu, Nov 12, 2020 at 10:23 AM Hongtao Liu <crazy...@gmail.com> wrote:
>
> On Thu, Nov 12, 2020 at 5:15 PM Hongtao Liu <crazy...@gmail.com> wrote:
> >
> > On Thu, Nov 12, 2020 at 5:12 PM Hongtao Liu <crazy...@gmail.com> wrote:
> > >
> > > On Thu, Nov 12, 2020 at 4:21 PM Uros Bizjak <ubiz...@gmail.com> wrote:
> > > >
> > > > On Thu, Nov 12, 2020 at 3:04 AM Hongtao Liu <crazy...@gmail.com> wrote:
> > > >
> > > > > > > gcc/ChangeLog:
> > > > > > >
> > > > > > > PR target/97194
> > > > > > > * config/i386/i386-expand.c (ix86_expand_vector_set_var): New
> > > > > > > function.
> > > > > > > * config/i386/i386-protos.h (ix86_expand_vector_set_var): New
> > > > > > > Decl.
> > > > > > > * config/i386/predicates.md (vec_setm_operand): New predicate,
> > > > > > > true for const_int_operand or register_operand under TARGET_AVX2.
> > > > > > > * config/i386/sse.md (vec_set<mode>): Support both constant
> > > > > > > and variable index vec_set.
> > > > > > >
> > > > > > > gcc/testsuite/ChangeLog:
> > > > > > >
> > > > > > > * gcc.target/i386/avx2-vec-set-1.c: New test.
> > > > > > > * gcc.target/i386/avx2-vec-set-2.c: New test.
> > > > > > > * gcc.target/i386/avx512bw-vec-set-1.c: New test.
> > > > > > > * gcc.target/i386/avx512bw-vec-set-2.c: New test.
> > > > > > > * gcc.target/i386/avx512f-vec-set-2.c: New test.
> > > > > > > * gcc.target/i386/avx512vl-vec-set-2.c: New test.
> > > > > >
> > > > > > +;; True for registers, or const_int_operand, used to vec_setm
> > > > > > expander.
> > > > > > +(define_predicate "vec_setm_operand"
> > > > > > + (ior (and (match_operand 0 "register_operand")
> > > > > > + (match_test "TARGET_AVX2"))
> > > > > > + (match_code "const_int")))
> > > > > > +
> > > > > > ;; True for registers, or 1 or -1. Used to optimize double-word
> > > > > > shifts.
> > > > > > (define_predicate "reg_or_pm1_operand"
> > > > > > (ior (match_operand 0 "register_operand")
> > > > > > diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md
> > > > > > index b153a87fb98..1798e5dea75 100644
> > > > > > --- a/gcc/config/i386/sse.md
> > > > > > +++ b/gcc/config/i386/sse.md
> > > > > > @@ -8098,11 +8098,14 @@ (define_insn "vec_setv2df_0"
> > > > > > (define_expand "vec_set<mode>"
> > > > > > [(match_operand:V 0 "register_operand")
> > > > > > (match_operand:<ssescalarmode> 1 "register_operand")
> > > > > > - (match_operand 2 "const_int_operand")]
> > > > > > + (match_operand 2 "vec_setm_operand")]
> > > > > >
> > > > > > You need to specify a mode, otherwise a register of any mode can
> > > > > > pass here.
> > > > > >
> > > > > Yes, theoretically, we only accept integer types. But in
> > > > > can_vec_set_var_idx_p
> > > > > cut
> > > > > ---
> > > > > bool
> > > > > can_vec_set_var_idx_p (machine_mode vec_mode)
> > > > > {
> > > > > if (!VECTOR_MODE_P (vec_mode))
> > > > > return false;
> > > > >
> > > > > machine_mode inner_mode = GET_MODE_INNER (vec_mode);
> > > > > rtx reg1 = alloca_raw_REG (vec_mode, LAST_VIRTUAL_REGISTER + 1);
> > > > > rtx reg2 = alloca_raw_REG (inner_mode, LAST_VIRTUAL_REGISTER + 2);
> > > > > rtx reg3 = alloca_raw_REG (VOIDmode, LAST_VIRTUAL_REGISTER + 3);
> > > > >
> > > > > enum insn_code icode = optab_handler (vec_set_optab, vec_mode);
> > > > >
> > > > > return icode != CODE_FOR_nothing && insn_operand_matches (icode, 0,
> > > > > reg1)
> > > > > && insn_operand_matches (icode, 1, reg2)
> > > > > && insn_operand_matches (icode, 2, reg3);
> > > > > }
> > > > > ---
> > > > >
> > > > > reg3 is assumed to be VOIDmode, set anymode in match_operand 2 will
> > > > > fail insn_operand_matches (icode, 2, reg3)
> > > > > ---
> > > > > (gdb) p insn_operand_matches(icode,2,reg3)
> > > > > $5 = false
> > > > > (gdb)
> > > > > ---
> > > > >
> > > > > Maybe we need to change
> > > > >
> > > > > rtx reg3 = alloca_raw_REG (VOIDmode, LAST_VIRTUAL_REGISTER + 3);
> > > > >
> > > > > to
> > > > >
> > > > > rtx reg3 = alloca_raw_REG (SImode, LAST_VIRTUAL_REGISTER + 3);
> > > > >
> > > > > cc Richard Biener, any thoughts?
> > > >
> > > > There are two targets (gcn in gcn-valu.md and s390 in vector.md) that
> > > > specify SImode for operand 2 in vec_setM pattern and allow register
> > > > operands. I wonder if and how they manage to generate the pattern.
> > > >
> > > > Uros.
> > >
> > > Variable index vec_set is enabled by r11-3486, about two months ago in
> > > [1]. But for the upper two targets, the codes are already there since
> > > GCC10(maybe earlier, i just looked at gcc10 branch), I don't think
> > > those codes are for [1].
> > >
> > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555905.html
> > >
> > >
> > > --
> > > BR,
> > > Hongtao
> >
> > Correct [1]
> > https://gcc.gnu.org/pipermail/gcc-patches/2020-September/554240.html
> >
> > --
> > BR,
> > Hongtao
>
> in https://gcc.gnu.org/pipermail/gcc-patches/2020-September/554592.html
>
> It says
>
> > >> +can_vec_set_var_idx_p (enum tree_code code, machine_mode vec_mode,
> > >> + machine_mode value_mode, machine_mode idx_mode)
> > >
> > > toplevel comment missing
> > >
> > >> +{
> > >> + gcc_assert (code == VECTOR_TYPE);
> > >
> > > what's the point of pasing 'code' here then? Since the optab only has a
> > > single
> > > mode, the vector mode, the value_mode is redundant as well. And I guess
> > > we might want to handle "arbitrary" index modes? That is, the .md
> > > expanders
> > > should not restrict its mode - I guess it simply uses VOIDmode at the
> > > moment
> > > (for integer constants). Not sure how to best do this without an
> > > explicit mode
> > > in the optab ...
> >
> > Yes, removed 'code' and value_mode by checking VECTOR_MODE_P and use
> > GET_MODE_INNER
> > for value_mode. ".md expanders" shall support for integer constants index
> > mode, but
> > I guess they shouldn't be expanded by IFN as this function is for variable
> > index
> > insert only? Anyway, the v3 patch used VOIDmode check...
I'm not sure what best to do here, as said accepting "any" (integer) mode as
input is desirable (SImode, DImode but eventually also smaller modes). How
that can be best achieved I don't know.
Why's not specifying any mode in the patter no good? Just make sure you
appropriately extend/subreg it? We can make sure it will be an integer
mode in the expander itself.
Richard.
>
> --
> BR,
> Hongtao
