[PATCH] Disparage slightly the mask register alternative for bitwise operations. [PR target/101142]

[Uros Bizjak]

On Wed, Jun 23, 2021 at 11:41 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Wed, Jun 23, 2021 at 11:32 AM Hongtao Liu <crazylht@gmail.com> wrote:
>
> > > > > > > Also when allocano cost of GENERAL_REGS is same as MASK_REGS, allocate
> > > > > > > MASK_REGS first since it has already been disparaged.
> > > > > > >
> > > > > > > gcc/ChangeLog:
> > > > > > >
> > > > > > >         PR target/101142
> > > > > > >         * config/i386/i386.md: (*anddi_1): Disparage slightly the mask
> > > > > > >         register alternative.
> > > > > > >         (*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.
> > > > > > >         * config/i386/i386.c (x86_order_regs_for_local_alloc): Change
> > > > > > >         the order of mask registers to be before general registers.
> > > > > > >
> > > > > > > gcc/testsuite/ChangeLog:
> > > > > > >
> > > > > > >         PR target/101142
> > > > > > >         * gcc.target/i386/spill_to_mask-1.c: Adjust testcase.
> > > > > > >         * gcc.target/i386/spill_to_mask-2.c: Adjust testcase.
> > > > > > >         * gcc.target/i386/spill_to_mask-3.c: Adjust testcase.
> > > > > > >         * gcc.target/i386/spill_to_mask-4.c: Adjust testcase.
> > > > > >
> > > > > > OK with a comment addition, see inline.
> > > > > >
> > > > > > Thanks,
> > > > > > Uros.
> > > > > >
> > > > > > > ---
> > > > > > >  gcc/config/i386/i386.c                        |  8 +-
> > > > > > >  gcc/config/i386/i386.md                       | 20 ++---
> > > > > > >  .../gcc.target/i386/spill_to_mask-1.c         | 89 +++++++++++++------
> > > > > > >  .../gcc.target/i386/spill_to_mask-2.c         | 11 ++-
> > > > > > >  .../gcc.target/i386/spill_to_mask-3.c         | 11 ++-
> > > > > > >  .../gcc.target/i386/spill_to_mask-4.c         | 11 ++-
> > > > > > >  6 files changed, 91 insertions(+), 59 deletions(-)
> > > > > > >
> > > > > > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> > > > > > > index a61255857ff..a651853ca3b 100644
> > > > > > > --- a/gcc/config/i386/i386.c
> > > > > > > +++ b/gcc/config/i386/i386.c
> > > > > > > @@ -20463,6 +20463,10 @@ x86_order_regs_for_local_alloc (void)
> > > > > > >     int pos = 0;
> > > > > > >     int i;
> > > > > > >
> > > > > > > +   /* Mask register.  */
> > > > > > > +   for (i = FIRST_MASK_REG; i <= LAST_MASK_REG; i++)
> > > > > > > +     reg_alloc_order [pos++] = i;
> > > > > >
> > > > > > Please add a comment why mask registers should come first.
> > > > > Thanks for the review, this is the patch i'm check in.
> > > >
> > > > This patch again caused unwanted mask instructions with -m32 in cpuid
> > > > check code, e.g.:
> > > >
> > > > Running target unix/-m32
> > > > FAIL: gcc.target/i386/avx512bw-pr70329-1.c execution test
> > > > FAIL: gcc.target/i386/pr96814.c execution test
> > > >
> > > > Debugging pr96814 failure:
> > > >
> > > >   0x0804921d <+66>:    mov    %edx,%ecx
> > > >   0x0804921f <+68>:    cpuid
> > > > => 0x08049221 <+70>:    kmovd  %edx,%k0
> > > >   0x08049225 <+74>:    mov    %eax,-0x8(%ebp)
> > > >   0x08049228 <+77>:    mov    %ebx,-0xc(%ebp)
> > > >
> > > > It looks to me that putting mask registers in front of GPR is looking
> > > So it's not functionality but performance issue here, under 32-bit
> > > mode there are only 8 gprs which result in higher register pressure,
> > > and for this we do have mask->integer and integer->mask cost, with
> > > -mtune=bdver1 where cost of kmovd is quite high(16, 20 /*
> > > mask->integer and integer->mask moves */), there's no mask
> > > instructions in cpuid.
> > > I guess we can adjust mask->integer and integer->mask for 32-bit mode
> > > to avoid such a situation?
> > I notice the default option is O0, with -O there's no mask instructions.
> > IMHO, We don't need to change cost unless there's -O2 cases where mask
> > instructions regress performance here.
>
> No, this reasoning is not acceptable. The compiled code will SIGILL on
> targets where unsupported mask registers are involved, so GPR should
> always have priority compared to mask registers. Based on these
> findings, x86_order_regs_for_local_alloc change should be reverted,
> and register move costs compensated elsewhere.

Longterm, I think that introducing vector BImode and VxBI vectors
should solve this issue. This would make a clear cut between mask and
GPR operations. I think that generic vector infrastructure supports
BImode vectors, so using e.g. "a & b" instead of builtins should work
well, too.

This approach would also avoid costly moves between register sets.

Uros.