Re: [PATCH v2] x86: make VPTERNLOG* usable on less than 512-bit operands with just AVX512F

[Hongtao Liu <crazylht@gmail.com>]

On Mon, Jun 19, 2023 at 3:09 PM Jan Beulich via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 19.06.2023 04:07, Liu, Hongtao wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Friday, June 16, 2023 2:22 PM
> >>
> >> --- a/gcc/config/i386/sse.md
> >> +++ b/gcc/config/i386/sse.md
> >> @@ -12597,11 +12597,11 @@
> >>     (set_attr "mode" "<sseinsnmode>")])
> >>
> >>  (define_insn "*<avx512>_vternlog<mode>_all"
> >> -  [(set (match_operand:V 0 "register_operand" "=v")
> >> +  [(set (match_operand:V 0 "register_operand" "=v,v")
> >>      (unspec:V
> >> -      [(match_operand:V 1 "register_operand" "0")
> >> -       (match_operand:V 2 "register_operand" "v")
> >> -       (match_operand:V 3 "bcst_vector_operand" "vmBr")
> >> +      [(match_operand:V 1 "register_operand" "0,0")
> >> +       (match_operand:V 2 "register_operand" "v,v")
> >> +       (match_operand:V 3 "bcst_vector_operand" "vBr,m")
> >>         (match_operand:SI 4 "const_0_to_255_operand")]
> >>        UNSPEC_VTERNLOG))]
> >>    "TARGET_AVX512F
> > Change condition to <MODE_SIZE> == 64 || TARGET_AVX512VL || (TARGET_AVX512F && !TARGET_PREFER_AVX256)
>
> May I ask why you think this is necessary? The condition of the insn
> already wasn't in sync with the condition used in all three splitters,
I think it's a latent bug for the original
*<avx512>_vternlog<mode>_all, it should be <MODE_SIZE> == 64 ||
TARGET_AVX512VL instead of TARGET_AVX512F, since TARGET_AVX512VL is
needed for 128/256-bit vectors. The bug won't be exposed since the
pattern is only generated by those 3 splitters which are guarded by
TARGET_AVX512VL. But We can just fix this to make the pattern exactly
correct.
> and I didn't see any reason why now they would need to be brought in
> sync. First and foremost because of the use of the UNSPEC (equally
> before and after this patch).
>
> Furthermore, isn't it the case that I'm already mostly expressing
> this with the "enabled" attribute? At the very least I think I
> should drop that again then if following your request?
You only handle alternative 1, but for alternative 0, it is still
enabled when TARGET_PREFER_AVX256 && !TARGET_AVX512VL for 128/256-bit
vectors. You don't need the drop that, alternative 1 still needs
<MODE_SIZE> == 64 || TARGET_AVX512VL since memory_operand can't be
operated with the zmm instruction for 128/256-bit vectors.
>
> > Also please add a testcase for case TARGET_AVX512F && !TARGET_PREFER_AVX256.
>
> Especially in a case like this one I'm wondering about the usefulness
> of a contrived testcase: It won't test more than one minor sub-case of
> the whole set of constructs covered here. But well, here as well as
> for the other change I'll invent something.
We don't need all sub-case, one is enough to guard that your
optimization won't be corrupted by a later commit.
.i.e
typedef int v4si __attribute((vector_size(16)));

v4si
foo (v4si a, v4si b, v4si c)
{
    return (a & b) | c;
}

We can now generate vpternlog with -mavx512f -O2 -mno-avx512vl
-mprefer-vector-width=512.

>
> Jan



-- 
BR,
Hongtao