On Wed, Aug 19, 2020 at 3:05 PM Uros Bizjak <ubiz...@gmail.com> wrote:
>
> On Wed, Aug 19, 2020 at 4:25 AM Hongtao Liu <crazy...@gmail.com> wrote:
> >
> > On Mon, Aug 17, 2020 at 6:08 PM Uros Bizjak <ubiz...@gmail.com> wrote:
> > >
> > > On Fri, Aug 14, 2020 at 10:26 AM Hongtao Liu <crazy...@gmail.com> wrote:
> > > >
> > > > Enable operator or/xor/and/andn/not for mask register, kxnor is not
> > > > enabled since there's no corresponding instruction for general
> > > > registers.
> > > >
> > > > gcc/
> > > > PR target/88808
> > > > * config/i386/i386.md: (*movsi_internal): Adjust constraints
> > > > for mask registers.
> > > > (*movhi_internal): Ditto.
> > > > (*movqi_internal): Ditto.
> > > > (*anddi_1): Support mask register operations
> > > > (*and<mode>_1): Ditto.
> > > > (*andqi_1): Ditto.
> > > > (*andn<mode>_1): Ditto.
> > > > (*<code><mode>_1): Ditto.
> > > > (*<code>qi_1): Ditto.
> > > > (*one_cmpl<mode>2_1): Ditto.
> > > > (*one_cmplsi2_1_zext): Ditto.
> > > > (*one_cmplqi2_1): Ditto.
> > > >
> > > > gcc/testsuite/
> > > > * gcc.target/i386/bitwise_mask_op-1.c: New test.
> > > > * gcc.target/i386/bitwise_mask_op-2.c: New test.
> > > > * gcc.target/i386/avx512bw-kunpckwd-1.c: Adjust testcase.
> > > > * gcc.target/i386/avx512bw-kunpckwd-3.c: Ditto.
> > > > * gcc.target/i386/avx512dq-kmovb-5.c: Ditto.
> > > > * gcc.target/i386/avx512f-kmovw-5.c: Ditto.
> > >
> > > index 74d207c3711..e8ad79d1b0a 100644
> > > --- a/gcc/config/i386/i386.md
> > > +++ b/gcc/config/i386/i386.md
> > > @@ -2294,7 +2294,7 @@
> > >
> > > (define_insn "*movsi_internal"
> > > [(set (match_operand:SI 0 "nonimmediate_operand"
> > > - "=r,m ,*y,*y,?*y,?m,?r,?*y,*v,*v,*v,m ,?r,?*v,*k,*k ,*rm,*k")
> > > + "=r,m ,*y,*y,?*y,?m,?r,?*y,*v,*v,*v,m ,?r,?*v,*k,*k ,*rm,k")
> > > (match_operand:SI 1 "general_operand"
> > > "g ,re,C ,*y,m ,*y,*y,r ,C ,*v,m ,*v,*v,r ,*r,*km,*k ,CBC"))]
> > > "!(MEM_P (operands[0]) && MEM_P (operands[1]))"
> > >
> > > I'd rather see *k everywhere, also with *movqi_internal and
> > > *movhi_internal patterns. The "*" means that the allocator won't
> > > allocate a mask register by default, but it will be used to optimize
> > > moves. With the above change, you are risking that during integer
> > > register pressure, the register allocator will allocate zero to a mask
> > > register, and later "optimize" the move with a direct maskreg-intreg
> > > move.
> > >
> > > The current strategy is that only general registers get allocated for
> > > integer modes. Let's keep it this way for now.
> > >
> >
> > Yes, though it would fail gcc.target/i386/avx512dq-pr88465.c and
> > gcc.target/i386/avx512f-pr88465.c, i think it's more reasonable not to
> > move zero into mask register directly.
>
> Although it would be nice if the register allocator was smart enough,
> the current strategy is to introduce peephole2 patterns to fix these
> problems, similar to [1]. These peepholes can be introduced in a
> follow-up patch.
>
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551744.html
>
peephole2 added.
> > > Otherwise, the patchset LGTM, but please test the suggested changes and
> > > repost.
> > >
> > > BTW: Do you plan to remove mask operations from sse.md? ATM, they are
> > > used to distinguish mask operations, generated from builtins from
> > > generic operations, so I'd like to keep them for a while. The drawback
> > > is, that they are not combined with other operations, but at the end
> > > of the day, this is what the programmer asked for by using builtins.
> >
> > Agree, I prefer to keep them.
>
> Thinking some more about the approach, it looks to me that the optimal
> solution is a post-reload splitter that would convert "generic"
> patterns to mask operations from sse.md. The mask operations don't set
> flags, so we can substantially improve post reload scheduling of these
> instructions by removing flags clobber.
>
> So, simply add "#" to relevant alternatives of logic patterns and add
> something like:
>
> --cut here--
> diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md
> index 41c6dbfa668..ad49bdc7583 100644
> --- a/gcc/config/i386/sse.md
> +++ b/gcc/config/i386/sse.md
> @@ -1470,6 +1470,18 @@
> ]
> (const_string "<MODE>")))])
>
> +(define_split
> + [(set (match_operand:SWI1248_AVX512BW 0 "mask_reg_operand")
> + (any_logic:SWI1248_AVX512BW
> + (match_operand:SWI1248_AVX512BW 1 "mask_reg_operand")
> + (match_operand:SWI1248_AVX512BW 2 "mask_reg_operand")))
> + (clobber (reg:CC FLAGS_REG))]
> + "TARGET_AVX512F && reload_completed"
> + [(parallel
> + [(set (match_dup 0)
> + (any_logic:SWI1248_AVX512BW (match_dup 1) (match_dup 2)))
> + (unspec [(const_int 0)] UNSPEC_MASKOP)])])
> +
> (define_insn "kandn<mode>"
> [(set (match_operand:SWI1248_AVX512BW 0 "register_operand" "=k")
> (and:SWI1248_AVX512BW
> --cut here--
>
> and similar for kandn and knot in sse.md. You will have to add
> mask_reg_operand predicate, see e.g. sse_reg_operand in predicates.md
> for example.
>
> We don't lose anything, because all important transformations,
> propagations and simplifications with these patterns happen before
> reload.
define_splits are added for those bitwise operations.
>
> Uros.
Also add bellow part which will pass gcc.target/i386/bitwise_mask_op-3.c
- must go into Q_REGS. */
+ must go into Q_REGS or ALL_MASK_REGS. */
if (GET_MODE (x) == QImode && !CONSTANT_P (x))
{
if (Q_CLASS_P (regclass))
return regclass;
else if (reg_class_subset_p (Q_REGS, regclass))
return Q_REGS;
+ else if (MASK_CLASS_P (regclass))
+ return regclass;
else
return NO_REGS;
Update patch.
--
BR,
Hongtao
