Re: [PATCH] Optimize %k register comparison against zero (PR target/82855)

[Uros Bizjak <ubizjak at gmail dot com>]

On Tue, Nov 7, 2017 at 11:47 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Nov 07, 2017 at 11:21:15AM +0100, Uros Bizjak wrote:
>> On Tue, Nov 7, 2017 at 11:10 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> >> > Isn't it sufficient that moves disparage slightly the k alternatives?
>> >> > Or are you worried about the case where the pseudo would need to be spilled
>> >> > and LRA would choose to reload it into a %kN register?
>> >>
>> >> The later... Perhaps it is possible to prevent unwanted reloads with
>> >> ?k (or even ??k, we already have ?m<r>, IDK). I have used *k in
>> >> zero-extend patterns to prevent unwanted reloads, which is perhaps too
>> >> strong, but we have ree pass that eliminates unwanted moves in this
>> >> case (*k in move patterns is handled by other regmove elimination
>> >> passes).
>> >
>> > ?k gives the same IRA decisions as *k, in both cases it considers
>> > GENERAL_REGS as expensive as MASK_EVEX_REGS and picks up GENERAL_REGS
>> > rather than MASK_EVEX_REGS for the pseudo, which results in
>> > kmovb %k1, %eax; test.
>>
>> Can we use some of the new RA modifiers here?
>
> So, for the testcase in question, GENERAL_REGS have cost 2000:
> k       works   (MASK_EVEX_REGS cost 0)
> *k      doesn't work, IRA chooses GENERAL_REGS (MASK_EVEX_REGS cost 2000)
> ?k      doesn't work, IRA chooses GENERAL_REGS (MASK_EVEX_REGS cost 2000)
> !k      works   (MASK_EVEX_REGS cost 0)
> ^k      doesn't work, IRA chooses GENERAL_REGS (MASK_EVEX_REGS cost 2000)
> $k      works   (MASK_EVEX_REGS cost 0)
>
>> --cut here--
>> '?'
>>      Disparage slightly the alternative that the '?' appears in, as a
>>      choice when no alternative applies exactly.  The compiler regards
>>      this alternative as one unit more costly for each '?' that appears
>>      in it.
>>
>> '!'
>>      Disparage severely the alternative that the '!' appears in.  This
>>      alternative can still be used if it fits without reloading, but if
>>      reloading is needed, some other alternative will be used.
>>
>> '^'
>>      This constraint is analogous to '?' but it disparages slightly the
>>      alternative only if the operand with the '^' needs a reload.
>>
>> '$'
>>      This constraint is analogous to '!' but it disparages severely the
>>      alternative only if the operand with the '$' needs a reload.
>> --cut here--
>>
>> '$' looks promising to prevent unwanted reloads to kN register.
>
> So, ok for trunk if it passes bootstrap/regtest?
>
> 2017-11-07  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/82855
>         * config/i386/i386.md (SWI1248_AVX512BWDQ2_64): New mode iterator.
>         (*cmp<mode>_ccz_1): New insn with $k alternative.
>
>         * gcc.target/i386/avx512dq-pr82855.c: New test.

Hopefully OK.

Thanks,
Uros.

> --- gcc/config/i386/i386.md.jj  2017-11-06 17:23:10.674996727 +0100
> +++ gcc/config/i386/i386.md     2017-11-07 11:44:40.081328781 +0100
> @@ -1275,6 +1275,26 @@ (define_expand "cmp<mode>_1"
>         (compare:CC (match_operand:SWI48 0 "nonimmediate_operand")
>                     (match_operand:SWI48 1 "<general_operand>")))])
>
> +(define_mode_iterator SWI1248_AVX512BWDQ2_64
> +  [(QI "TARGET_AVX512DQ") (HI "TARGET_AVX512DQ")
> +   (SI "TARGET_AVX512BW") (DI "TARGET_AVX512BW && TARGET_64BIT")])
> +
> +(define_insn "*cmp<mode>_ccz_1"
> +  [(set (reg FLAGS_REG)
> +       (compare (match_operand:SWI1248_AVX512BWDQ2_64 0
> +                       "nonimmediate_operand" "<r>,?m<r>,$k")
> +                (match_operand:SWI1248_AVX512BWDQ2_64 1 "const0_operand")))]
> +  "ix86_match_ccmode (insn, CCZmode)"
> +  "@
> +   test{<imodesuffix>}\t%0, %0
> +   cmp{<imodesuffix>}\t{%1, %0|%0, %1}
> +   ktest<mskmodesuffix>\t%0, %0"
> +  [(set_attr "type" "test,icmp,msklog")
> +   (set_attr "length_immediate" "0,1,*")
> +   (set_attr "modrm_class" "op0,unknown,*")
> +   (set_attr "prefix" "*,*,vex")
> +   (set_attr "mode" "<MODE>")])
> +
>  (define_insn "*cmp<mode>_ccno_1"
>    [(set (reg FLAGS_REG)
>         (compare (match_operand:SWI 0 "nonimmediate_operand" "<r>,?m<r>")
> --- gcc/testsuite/gcc.target/i386/avx512dq-pr82855.c.jj 2017-11-07 08:56:02.910321163 +0100
> +++ gcc/testsuite/gcc.target/i386/avx512dq-pr82855.c    2017-11-07 08:56:02.910321163 +0100
> @@ -0,0 +1,14 @@
> +/* PR target/82855 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mavx512vl -mavx512dq" } */
> +/* { dg-final { scan-assembler {\mktestb\M} } } */
> +
> +#include <immintrin.h>
> +
> +int
> +foo (const __m256i *ptr)
> +{
> +  __m256i v = _mm256_loadu_si256 (ptr);
> +  __mmask8 m = _mm256_cmpeq_epi32_mask (v, _mm256_setzero_si256 ());
> +  return 0 == m;
> +}
>
>
>         Jakub