x86: Support Intel AVX VNNI

[H.J. Lu]

On Thu, Oct 15, 2020 at 9:04 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.10.2020 17:34, H.J. Lu wrote:
> > On Thu, Oct 15, 2020 at 8:28 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 15.10.2020 17:23, H.J. Lu wrote:
> >>> On Thu, Oct 15, 2020 at 8:22 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 15.10.2020 14:38, H.J. Lu wrote:
> >>>>> On Thu, Oct 15, 2020 at 5:28 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>
> >>>>>> On 15.10.2020 13:15, H.J. Lu wrote:
> >>>>>>> On Thu, Oct 15, 2020 at 12:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>> On 15.10.2020 09:10, Cui, Lili wrote:
> >>>>>>>>>>>> @@ -1964,7 +1967,14 @@ cpu_flags_match (const insn_template *t)
> >>>>>>>>>>>>        cpu = cpu_flags_and (x, cpu);
> >>>>>>>>>>>>        if (!cpu_flags_all_zero (&cpu))
> >>>>>>>>>>>>       {
> >>>>>>>>>>>> -       if (x.bitfield.cpuavx)
> >>>>>>>>>>>> +       if (x.bitfield.cpuvex_prefix)
> >>>>>>>>>>>> +         {
> >>>>>>>>>>>> +           /* We need to check a few extra flags with VEX_PREFIX.  */
> >>>>>>>>>>>> +           if (i.vec_encoding == vex_encoding_vex
> >>>>>>>>>>>> +               || i.vec_encoding == vex_encoding_vex3)
> >>>>>>>>>>>> +             match |= CPU_FLAGS_ARCH_MATCH;
> >>>>>>>>>>>> +         }
> >>>>>>>>>>>> +       else if (x.bitfield.cpuavx)
> >>>>>>>>>>>
> >>>>>>>>>>> Is this (including the new cpuvex_prefix attribute, which imo
> >>>>>>>>>>> shouldn't be a Cpu* bit) really needed? Couldn't you achieve the same
> >>>>>>>>>>> by placing the templates _after_ the AVX512 counterparts? Iirc
> >>>>>>>>>>> templates get tried in order, and the first match wins. The {vex3}
> >>>>>>>>>>> prefix would then prevent a match on the EVEX-encoded AVX512_VNNI
> >>>>>>>>>> templates.
> >>>>>>>>>>
> >>>>>>>>>> Lili, please look into it.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I add an invalid test for it, we need cpuvex_prefix attribute for under scenario.
> >>>>>>>>>
> >>>>>>>>> .arch .noavx512_vnni
> >>>>>>>>> vpdpbusd %xmm2,%xmm4,%xmm2
> >>>>>>>>>
> >>>>>>>>> As without the pseudo {vex} prefix, this instruction should be encoded with EVEX prefix.
> >>>>>>>>> we should report error for it, I rename CpuVEX_PREFIX to PseudoVexPrefix
> >>>>>>>>> and move it into opcode_modifier bit, thanks.
> >>>>>>>>
> >>>>>>>> I disagree, unless AVX-VNNI was specified to have a dependency on
> >>>>>>>> AVX512-VNNI (which would seem pretty odd, as meanwhile I've noticed
> >>>>>>>> that another reason for introducing these encodings may be to allow
> >>>>>>>> their use on AVX512-incapable hardware). The above very much should
> >>>>>>>> result in the VEX encoding despite the absence of a {vex} prefix.
> >>>>>>>> It's really only the default case of everything being enabled where
> >>>>>>>> the pseudo-prefix should be mandated. This particularly implies
> >>>>>>>> that an explicit ".arch .avx_vnni" ought to _also_ eliminate the
> >>>>>>>> need for the pseudo prefix.
> >>>>>>>
> >>>>>>> AVX VNNI always requires the {vex} prefix.  It isn't optional.
> >>>>>>
> >>>>>> That's said or written where? These are new insns with - afaict - no
> >>>>>> specification beyond the ISA extensions doc. There's nothing like
> >>>>>
> >>>>> This is true.  When we implemented AVX VNNI, we decided that
> >>>>> the {vex} prefix is mandatory so that
> >>>>>
> >>>>> vpdpbusd %xmm2,%xmm4,%xmm2
> >>>>>
> >>>>> always mean EVEX encoding.
> >>>>
> >>>> And this decision was discussed internally at Intel, and other
> >>>
> >>> Internally.
> >>>
> >>>> community members get no say at all?
> >>
> >> Please can such discussions be had in the public for open source projects?
> >
> > The discussion happened around the time when I added
> >
> > commit 86fa6981e7487e2c2df4337aa75ed2d93c32eaf2
> > Author: H.J. Lu <hjl.tools@gmail.com>
> > Date:   Thu Mar 9 09:58:46 2017 -0800
> >
> >     X86: Add pseudo prefixes to control encoding
> >
> >     Many x86 instructions have more than one encodings.  Assembler picks
> >     the default one, usually the shortest one.  Although the ".s", ".d8"
> >     and ".d32" suffixes can be used to swap register operands or specify
> >     displacement size, they aren't very flexible.  This patch adds pseudo
> >     prefixes, {xxx}, to control instruction encoding.  The available
> >     pseudo prefixes are {disp8}, {disp32}, {load}, {store}, {vex2}, {vex3}
> >     and {evex}.  Pseudo prefixes are preferred over the ".s", ".d8" and
> >     ".d32" suffixes, which are deprecated.
> >
> > It wasn't practical to discuss it in public.
>
> Wow.
>
> >> I continue to think that the behavior as implemented is not the best
> >> possible choice. Therefore I'd like to at least hear the arguments that
> >> led to this decision.
> >
> > Please send me your detailed comments.  I will forward it to our internal
> > group.
>
> I've given my two points already - there are two cases where the
> pseudo prefix shouldn't be required. Plus, as also said, the
> disassembler shouldn't display it by default.
>

It needs to be much more than that to have any impact on a
decision made years ago.


-- 
H.J.