Re: x86: making better use of vpternlog{d,q}

[Hongtao Liu <crazylht@gmail.com>]

On Wed, May 24, 2023 at 3:58 PM Jan Beulich via Gcc <gcc@gcc.gnu.org> wrote:
>
> Hello,
>
> for a couple of years I was meaning to extend the use of these AVX512F
> insns beyond the pretty minimalistic ones there are so far. Now that I've
> got around to at least draft something, I ran into a couple of issues I
> cannot explain. I'd like to start with understanding the unexpected
> effects of a change to an existing insn I have made (reproduced at the
> bottom). I certainly was prepared to observe testsuite failures, but it
> ends up failing tests I didn't expect it would fail, and - upon looking
> at sibling ones - also ends up leaving intact tests which I would expect
> would then need adjustment (because of using the new alternative).
>
> In particular (all mentioned tests are in gcc.target/i386/)
> - avx512f-andn-si-zmm-1.c (and its AVX512VL counterparts) fails because
>   for whatever reason generated code reverts back to using vpbroadcastd,
> - avx512f-andn-di-zmm-1.c, otoh, is unaffected (i.e. continues to use
>   vpandnq with embedded broadcast),
> - avx512f-andn-si-zmm-2.c doesn't use the new 4th insn alternative when
>   at the same time a made-up DI variant of the test (akin to what might
>   be an avx512f-andn-di-zmm-2.c testcase) does.
> IOW: How is SI mode element size different here from DI mode one? Is
> there anything wrong with the 4th alternative I'm adding, or is this
> hinting at some anomaly elsewhere?
__m512i is defined as __v8di, when it's used for _mm512_andnot_epi32,
it's explicitlt converted to (__v16si) and creates an extra subreg
which is not needed for DImode cases.
And pass_combine try to match the below pattern but failed due to the
condition REG_P (operands[1]) || REG_P (operands[2]). Here I think you
want register_operand instead of REG_P.
157(set (reg:V16SI 91)
158    (and:V16SI (not:V16SI (subreg:V16SI (reg:V8DI 98) 0))
159        (vec_duplicate:V16SI (mem:SI (reg:DI 99) [1 *f_3(D)+0 S4 A32]))))


>
> Just to mention it, avx512f-andn-si-zmm-5.c similarly fails
> unexpectedly, but I guess for the same reason (and there aren't AVX512VL
> or DI mode element counterparts thereof).
>
> Jan
>
> --- a/gcc/config/i386/sse.md
> +++ b/gcc/config/i386/sse.md
> @@ -17019,11 +17019,11 @@
>    "TARGET_AVX512F")
>
>  (define_insn "*andnot<mode>3"
> -  [(set (match_operand:VI 0 "register_operand" "=x,x,v")
> +  [(set (match_operand:VI 0 "register_operand" "=x,x,v,v")
>         (and:VI
> -         (not:VI (match_operand:VI 1 "vector_operand" "0,x,v"))
> -         (match_operand:VI 2 "bcst_vector_operand" "xBm,xm,vmBr")))]
> -  "TARGET_SSE"
> +         (not:VI (match_operand:VI 1 "bcst_vector_operand" "0,x,v,mBr"))
> +         (match_operand:VI 2 "bcst_vector_operand" "xBm,xm,vmBr,v")))]
> +  "TARGET_SSE && (REG_P (operands[1]) || REG_P (operands[2]))"
>  {
>    char buf[64];
>    const char *ops;
> @@ -17090,6 +17090,11 @@
>      case 2:
>        ops = "v%s%s\t{%%2, %%1, %%0|%%0, %%1, %%2}";
>        break;
> +    case 3:
> +      tmp = "pternlog";
> +      ssesuffix = "<ternlogsuffix>";
> +      ops = "v%s%s\t{$0x44, %%1, %%2, %%0|%%0, %%2, %%1, $0x44}";
> +      break;
>      default:
>        gcc_unreachable ();
>      }
> @@ -17098,7 +17103,7 @@
>    output_asm_insn (buf, operands);
>    return "";
>  }
> -  [(set_attr "isa" "noavx,avx,avx")
> +  [(set_attr "isa" "noavx,avx,avx,avx512f")
>     (set_attr "type" "sselog")
>     (set (attr "prefix_data16")
>       (if_then_else
> @@ -17106,7 +17111,7 @@
>             (eq_attr "mode" "TI"))
>         (const_string "1")
>         (const_string "*")))
> -   (set_attr "prefix" "orig,vex,evex")
> +   (set_attr "prefix" "orig,vex,evex,evex")
>     (set (attr "mode")
>         (cond [(match_test "TARGET_AVX2")
>                  (const_string "<sseinsnmode>")
> @@ -17119,7 +17124,11 @@
>                     (match_test "optimize_function_for_size_p (cfun)"))
>                  (const_string "V4SF")
>               ]
> -             (const_string "<sseinsnmode>")))])
> +             (const_string "<sseinsnmode>")))
> +   (set (attr "enabled")
> +       (if_then_else (eq_attr "alternative" "3")
> +                     (symbol_ref "<MODE_SIZE> == 64 ? TARGET_AVX512F : TARGET_AVX512VL")
> +                     (const_string "*")))])
>
>  ;; PR target/100711: Split notl; vpbroadcastd; vpand as vpbroadcastd; vpandn
>  (define_split



-- 
BR,
Hongtao