Re: [PATCH] [PR target/97194] [AVX2] Support variable index vec_set.

[Uros Bizjak <ubizjak@gmail.com>]

On Thu, Nov 12, 2020 at 2:59 PM Richard Biener
<richard.guenther@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.

I was expecting something similar to how extvM/extzvM operands are
handled here. We have:

    Operands 0 and 1 both have mode M.  Operands 2 and 3 have a
    target-specific mode.

Please note operands 2 and 3 having a "target-specific" mode, handled
in optabs-query.c as:

  machine_mode struct_mode = data->operand[struct_op].mode;
  if (struct_mode == VOIDmode)
    struct_mode = word_mode;
  if (mode != struct_mode)
    return false;

> 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.

IIRC, having known mode, expanders can use create_convert_operand_to,
and the middle-end will do the above by itself. Also note that at
least two targets specify SImode, so register operands are currently
ineffective there.

Adding RichardS to CC for the expand part.

Uros.