Enable Intel AVX512_FP16 instructions and test
Cui, Lili
lili.cui@intel.com
Fri Jul 23 15:42:58 GMT 2021
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, July 22, 2021 8:25 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: hjl.tools@gmail.com; binutils@sourceware.org
> Subject: Re: Enable Intel AVX512_FP16 instructions and test
>
> On 22.07.2021 09:04, Cui, Lili wrote:
> > --- a/opcodes/i386-dis-evex-mod.h
> > +++ b/opcodes/i386-dis-evex-mod.h
> > @@ -87,3 +87,13 @@
> > /* MOD_EVEX_0F38C7 */
> > { EVEX_LEN_TABLE (EVEX_LEN_0F38C7_M_0) },
> > },
> > + {
> > + /* MOD_EVEX_MAP5_10_PREFIX_1 */
> > + { "vmovs%XH", { XMScalar, EXwS }, 0 },
>
> I don't think it's EXsS here, but EXw. The swapped forms are used in the
> other entry. See the respective VMOVSS and VMOVSD entries.
>
You are right, we shouldn't use swapped form here, the difference between EXw and here is Compressed displacement.
> > + { "vmovs%XH", { XMScalar, VexScalar, EXxmm_md }, 0 },
>
> EXxmm_md looks wrong to me (and the macro is now gone anyway), even if
> just in a benign way. In new terms I think you would mean EXw, but
> considering you've already separated reg from mem here you really want EXx,
> quite like ....
>
> > + },
> > + {
> > + /* MOD_EVEX_MAP5_11_PREFIX_1 */
> > + { "vmovs%XH", { EXwS, XMScalar }, 0 },
> > + { "vmovs%XH", { EXxS, Vex, XMScalar }, 0 },
>
> ... your use of EXxS here. In turn instead of Vex here I think you mean
> VexScalar (like you have above). But then compare what you have to
> VMOVSS and VMOVSD again: There decoding doesn't use ModR/M.mod at
> all, but instead uses VexScalarR as middle operand.
> Please follow that pattern unless there's a clear reason not to.
>
Done.
> > @@ -646,3 +646,66 @@
> > { Bad_Opcode },
> > { "vpshrdw", { XM, Vex, EXx, Ib }, 0 },
> > },
> > + /* EVEX_W_MAP5_1D_P_2 */
> > + {
> > + { "vcvtps2phx%XY", { XMxmmq, EXx, EXxEVexR }, 0 },
>
> "vcvtps2p%XHx%XY" would allow this to move to i386-dis-evex-prefix.h.
> Similarly for the majority of further entries here, or perhaps even all of them,
> as ...
>
Done.
> > + /* EVEX_W_MAP5_5A */
> > + {
> > + { PREFIX_TABLE (PREFIX_EVEX_MAP5_5A_W_0) },
> > + { PREFIX_TABLE (PREFIX_EVEX_MAP5_5A_W_1) }, },
> > + /* EVEX_W_MAP5_5B */
> > + {
> > + { PREFIX_TABLE (PREFIX_EVEX_MAP5_5B_W_0) },
> > + { PREFIX_TABLE (PREFIX_EVEX_MAP5_5B_W_1) }, },
>
> ... even these and ...
>
> > + /* EVEX_W_MAP5_7D */
> > + {
> > + { PREFIX_TABLE (PREFIX_EVEX_MAP5_7D_W_0) }, },
>
> ... this could be handled the same way without going through EVEX.W
> decoding.
>
I think EVEX_W_MAP5_5A/5B/7D are different from the previous one, they are a bit more complicated. I think it is better to go through EVEX.W first, then go through prefix table.
> > --- a/opcodes/i386-dis-evex.h
> > +++ b/opcodes/i386-dis-evex.h
> > @@ -403,8 +403,8 @@ static const struct dis386 evex_table[][256] = {
> > /* 60 */
> > { Bad_Opcode },
> > { Bad_Opcode },
> > - { "vpexpand%BW", { XM, EXbwUnit }, PREFIX_DATA },
> > - { "vpcompress%BW", { EXbwUnit, XM }, PREFIX_DATA },
> > + { "vpexpand%BW", { XM, EXbwUnit }, PREFIX_DATA },
> > + { "vpcompress%BW", { EXbwUnit, XM }, PREFIX_DATA },
> > { "vpblendm%DQ", { XM, Vex, EXx }, PREFIX_DATA },
> > { "vblendmp%XW", { XM, Vex, EXx }, PREFIX_DATA },
> > { "vpblendm%BW", { XM, Vex, EXx }, PREFIX_DATA },
> > @@ -453,7 +453,7 @@ static const struct dis386 evex_table[][256] = {
> > { Bad_Opcode },
> > { "vperm%BW", { XM, Vex, EXx }, PREFIX_DATA },
> > { Bad_Opcode },
> > - { "vpshufbitqmb", { XMask, Vex, EXx }, PREFIX_DATA },
> > + { "vpshufbitqmb", { XMask, Vex, EXx }, PREFIX_DATA },
> > /* 90 */
> > { "vpgatherd%DQ", { XMGatherD, MVexVSIBDWpX },
> PREFIX_DATA },
> > { "vpgatherq%DQ", { XMGatherQ, MVexVSIBQWpX },
> PREFIX_DATA },
> > @@ -518,11 +518,11 @@ static const struct dis386 evex_table[][256] = {
> > { MOD_TABLE (MOD_EVEX_0F38C6) },
> > { MOD_TABLE (MOD_EVEX_0F38C7) },
> > /* C8 */
> > - { "vexp2p%XW", { XM, EXx, EXxEVexS }, PREFIX_DATA },
> > + { "vexp2p%XW", { XM, EXx, EXxEVexS }, PREFIX_DATA },
> > { Bad_Opcode },
> > - { "vrcp28p%XW", { XM, EXx, EXxEVexS }, PREFIX_DATA },
> > + { "vrcp28p%XW", { XM, EXx, EXxEVexS }, PREFIX_DATA },
> > { "vrcp28s%XW", { XMScalar, VexScalar, EXVexWdqScalar, EXxEVexS },
> PREFIX_DATA },
> > - { "vrsqrt28p%XW", { XM, EXx, EXxEVexS }, PREFIX_DATA },
> > + { "vrsqrt28p%XW", { XM, EXx, EXxEVexS }, PREFIX_DATA },
> > { "vrsqrt28s%XW", { XMScalar, VexScalar, EXVexWdqScalar, EXxEVexS },
> PREFIX_DATA },
> > { Bad_Opcode },
> > { VEX_W_TABLE (VEX_W_0F38CF) },
>
> All of he above looks unrelated. Cleaning up whitespace use may be a good
> thing, but in the future I'd recommend to avoid such in already large patches.
After adding more instructions to the evex_table, I found that these instructions were not aligned with others, so I added taps for them, but in the mail format they look like Space. Do you want me to restore them?
>
> > @@ -562,6 +575,8 @@ enum
> >
> > /* scalar, ignore vector length. */
> > scalar_mode,
> > + /* like w_mode, ignore vector length. */ w_swap_mode,
>
> Any chance you could move this up next to {b,d,q}_swap_mode and also
> make its comment match those other ones?
>
Done.
> > @@ -865,7 +880,9 @@ enum
> > MOD_EVEX_0F387B_W_0,
> > MOD_EVEX_0F387C,
> > MOD_EVEX_0F38C6,
> > - MOD_EVEX_0F38C7
> > + MOD_EVEX_0F38C7,
> > + MOD_EVEX_MAP5_10_PREFIX_1,
> > + MOD_EVEX_MAP5_11_PREFIX_1
> > };
>
> Just P_1 please, not PREFIX_1. It's only very old enumerators which still use
> the overlong names.
>
They are vmovsh and are deleted.
> Also, to avoid the need to touch the previously last line when adding new
> enumerators, it is good practice to have a trailing comma.
> Like e.g. ..
>
> > @@ -1102,6 +1119,47 @@ enum
> > PREFIX_EVEX_0F389B,
> > PREFIX_EVEX_0F38AA,
> > PREFIX_EVEX_0F38AB,
> > + PREFIX_EVEX_0F3A08_W_0,
>
> ... was the case here before your additions.
>
Ok, I added a blank line between them. Thanks.
> > @@ -10323,7 +10407,6 @@ putop (const char *in_template, int sizeflag)
> > int cond = 1;
> > unsigned int l = 0, len = 0;
> > char last[4];
> > -
> > for (p = in_template; *p; p++)
> > {
> > if (len > l)
>
> I don't think we want that blank line removed.
>
Done.
> > @@ -10462,29 +10545,45 @@ putop (const char *in_template, int sizeflag)
> > used_prefixes |= (prefixes & PREFIX_DATA);
> > break;
> > case 'H':
> > - if (intel_syntax)
> > - break;
> > - if ((prefixes & (PREFIX_CS | PREFIX_DS)) == PREFIX_CS
> > - || (prefixes & (PREFIX_CS | PREFIX_DS)) == PREFIX_DS)
> > + if (l == 0)
> > {
> > - used_prefixes |= prefixes & (PREFIX_CS | PREFIX_DS);
> > - *obufp++ = ',';
> > - *obufp++ = 'p';
> > -
> > - /* Set active_seg_prefix even if not set in 64-bit mode
> > - because here it is a valid branch hint. */
> > - if (prefixes & PREFIX_DS)
> > + if (intel_syntax)
> > + break;
> > + if ((prefixes & (PREFIX_CS | PREFIX_DS)) == PREFIX_CS
> > + || (prefixes & (PREFIX_CS | PREFIX_DS)) == PREFIX_DS)
> > {
> > - active_seg_prefix = PREFIX_DS;
> > - *obufp++ = 't';
> > + used_prefixes |= prefixes & (PREFIX_CS | PREFIX_DS);
> > + *obufp++ = ',';
> > + *obufp++ = 'p';
> > +
> > + /* Set active_seg_prefix even if not set in 64-bit mode
> > + because here it is a valid branch hint. */
> > + if (prefixes & PREFIX_DS)
> > + {
> > + active_seg_prefix = PREFIX_DS;
> > + *obufp++ = 't';
> > + }
> > + else
> > + {
> > + active_seg_prefix = PREFIX_CS;
> > + *obufp++ = 'n';
> > + }
> > }
> > + }
> > + else if (l == 1 && last[0] == 'X')
> > + {
> > + if (vex.w == 0)
> > + *obufp++ = 'h';
> > else
> > {
> > - active_seg_prefix = PREFIX_CS;
> > - *obufp++ = 'n';
> > + obufp = obuf;
> > + BadOp ();
>
> As just said to H.J. in the context of my bugfix/cleanup series (which I'm
> afraid you'll need to re-base over), BadOp() has some really unhelpful
> properties. I will admit though that good alternatives aren't easy to come up
> with. What I'd like to avoid is that part of the insn bytes that are part of a
> proper encoding (which merely doesn't represent any valid insn) get taken as
> the start of the next insn. One variant might be to print "(bad)" in the place
> you're printing 'h' for EVEX.W clear. H.J. may have other preferences or
> suggestions ... But in particular ...
>
> > + mnemonicendp = obufp;
> > + codep = codep + 4;
>
> ... this isn't going to help afaict - 4 may be the correct value for register-only
> insns and memory operands without displacement and without SIB, but I
> think it'll be wrong for other ModR/M forms.
Yes, you are right, I changed it to only print {bad} instead of 'h'.
like this
vadds{bad} %xmm28,%xmm29,%xmm30
>
> > + return 0;
> > }
> > }
> > - break;
> > + break;
>
> I think you break indentation on this last line. I also think you want
>
> else
> abort ();
>
> immediately ahead of it, like we have elsewhere when handling multi-
> character sequences.
Yes, I added it.
>
> > @@ -10967,14 +11066,23 @@ print_displacement (char *buf, bfd_vma
> disp)
> > static void intel_operand_size (int bytemode, int sizeflag) {
> > - if (vex.b
> > - && (bytemode == x_mode
> > - || bytemode == evex_half_bcst_xmmq_mode))
> > + if (vex.b)
> > {
> > - if (vex.w)
> > - oappend ("QWORD PTR ");
> > - else
> > - oappend ("DWORD PTR ");
> > + switch (bytemode)
> > + {
> > + case x_mode:
> > + case evex_half_bcst_xmmq_mode:
>
> Preferably (imo) this would become just "default:"; alternatively you will
> want to ...
>
> > + if (vex.w)
> > + oappend ("QWORD PTR ");
> > + else
> > + oappend ("DWORD PTR ");
> > + break;
> > + case xh_mode:
> > + case evex_half_bcst_xmmqh_mode:
> > + case evex_half_bcst_xmmqdh_mode:
> > + oappend ("WORD PTR ");
> > + break;
>
> add
>
> default:
> abort ();
>
> here, like we do elsewhere.
Done.
>
> > @@ -11467,6 +11583,15 @@ OP_E_memory (int bytemode, int sizeflag)
> > case evex_x_gscat_mode:
> > shift = vex.w ? 3 : 2;
> > break;
> > + case xh_mode:
> > + case evex_half_bcst_xmmqh_mode:
> > + case evex_half_bcst_xmmqdh_mode:
> > + if (vex.b)
> > + {
> > + shift = vex.w ? 2 : 1;
>
> Aren't you speculating on the meaning of EVEX.W here? Unless I'm
> overlooking a case this covers, I think you want to abort() here when vex.w is
> set.
>
> > @@ -11881,9 +12008,36 @@ OP_E_memory (int bytemode, int sizeflag)
> > }
> > if (vex.b
> > && (bytemode == x_mode
> > + || bytemode == xh_mode
> > + || bytemode == evex_half_bcst_xmmqh_mode
> > + || bytemode == evex_half_bcst_xmmqdh_mode
> > || bytemode == evex_half_bcst_xmmq_mode))
> > {
> > - if (vex.w
> > + if (bytemode == xh_mode)
> > + {
> > + if (vex.w)
> > + {
> > + }
>
> Again
>
> abort ();
>
Done.
> would seem more consistent with what we do elsewhere (unless, like above,
> I'm overlooking a case).
>
> > @@ -374,7 +376,7 @@ static initializer cpu_flag_init[] =
> > { "CPU_ANY_AVX2_FLAGS",
> > "CPU_ANY_AVX512F_FLAGS|CpuAVX2" },
> > { "CPU_ANY_AVX512F_FLAGS",
> > -
> "CpuAVX512F|CpuAVX512CD|CpuAVX512ER|CpuAVX512PF|CpuAVX512DQ|
> CpuAVX512BW|CpuAVX512VL|CpuAVX512IFMA|CpuAVX512VBMI|CpuAVX5
> 12_4FMAPS|CpuAVX512_4VNNIW|CpuAVX512_VPOPCNTDQ|CpuAVX512_V
> BMI2|CpuAVX512_VNNI|CpuAVX512_BITALG|CpuAVX512_BF16|CpuAVX512
> _VP2INTERSECT" },
> > +
> > +
> "CpuAVX512F|CpuAVX512CD|CpuAVX512ER|CpuAVX512PF|CpuAVX512DQ|
> CpuAVX51
> > +
> 2VL|CpuAVX512IFMA|CpuAVX512VBMI|CpuAVX512_4FMAPS|CpuAVX512_4
> VNNIW|Cp
> > +
> uAVX512_VPOPCNTDQ|CpuAVX512_VBMI2|CpuAVX512_VNNI|CpuAVX512_
> BITALG|Cp
> > +
> uAVX512_BF16|CpuAVX512_VP2INTERSECT|CPU_ANY_AVX512BW_FLAGS" },
>
> Personally I would have replaced CpuAVX512BW by
> CPU_ANY_AVX512BW_FLAGS (as it should have been in the first place, as
> indicated before), not tagged the latter to the end. But anyway ...
>
Done.
Thanks,
Lili.
More information about the Binutils
mailing list