From: Jan Beulich <jbeulich@suse.com>
To: "Cui, Lili" <lili.cui@intel.com>, konglin1 <lingling.kong@intel.com>
Cc: hongjiu.lu@intel.com, ccoutant@gmail.com, binutils@sourceware.org
Subject: Re: [PATCH 5/8] Support APX NDD
Date: Wed, 8 Nov 2023 12:13:55 +0100 [thread overview]
Message-ID: <f72435bc-d117-8d4e-3391-cc597bf1a89a@suse.com> (raw)
In-Reply-To: <20231102112911.2372810-6-lili.cui@intel.com>
On 02.11.2023 12:29, Cui, Lili wrote:
> --- a/opcodes/i386-dis-evex-prefix.h
> +++ b/opcodes/i386-dis-evex-prefix.h
> @@ -338,10 +338,6 @@
> { "vcmpp%XH", { MaskG, Vex, EXxh, EXxEVexS, CMP }, 0 },
> { "vcmps%XH", { MaskG, VexScalar, EXw, EXxEVexS, CMP }, 0 },
> },
> - /* PREFIX_EVEX_MAP4_66 */
> - {
> - { "wrssK", { M, Gdq }, 0 },
> - },
> /* PREFIX_EVEX_MAP4_D8 */
> {
> { "sha1nexte", { XM, EXxmm }, 0 },
What's going on here?
> --- a/opcodes/i386-dis-evex-reg.h
> +++ b/opcodes/i386-dis-evex-reg.h
> @@ -56,6 +56,36 @@
> { "blsmskS", { VexGdq, Edq }, 0 },
> { "blsiS", { VexGdq, Edq }, 0 },
> },
> + /* REG_EVEX_MAP4_80 */
> + {
> + { "addA", { VexGb, Eb, Ib }, 0 },
> + { "orA", { VexGb, Eb, Ib }, 0 },
> + { "adcA", { VexGb, Eb, Ib }, 0 },
> + { "sbbA", { VexGb, Eb, Ib }, 0 },
> + { "andA", { VexGb, Eb, Ib }, 0 },
> + { "subA", { VexGb, Eb, Ib }, 0 },
> + { "xorA", { VexGb, Eb, Ib }, 0 },
> + },
> + /* REG_EVEX_MAP4_81 */
> + {
> + { "addQ", { VexGv, Ev, Iv }, 0 },
> + { "orQ", { VexGv, Ev, Iv }, 0 },
> + { "adcQ", { VexGv, Ev, Iv }, 0 },
> + { "sbbQ", { VexGv, Ev, Iv }, 0 },
> + { "andQ", { VexGv, Ev, Iv }, 0 },
> + { "subQ", { VexGv, Ev, Iv }, 0 },
> + { "xorQ", { VexGv, Ev, Iv }, 0 },
> + },
> + /* REG_EVEX_MAP4_83 */
> + {
> + { "addQ", { VexGv, Ev, sIb }, 0 },
> + { "orQ", { VexGv, Ev, sIb }, 0 },
> + { "adcQ", { VexGv, Ev, sIb }, 0 },
> + { "sbbQ", { VexGv, Ev, sIb }, 0 },
> + { "andQ", { VexGv, Ev, sIb }, 0 },
> + { "subQ", { VexGv, Ev, sIb }, 0 },
> + { "xorQ", { VexGv, Ev, sIb }, 0 },
> + },
No sign of prefix decoding, and also no PREFIX_OPCODE present?
> @@ -63,3 +93,27 @@
> { "aesencwide256kl", { M }, 0 },
> { "aesdecwide256kl", { M }, 0 },
> },
> + /* REG_EVEX_MAP4_F6 */
> + {
> + { Bad_Opcode },
> + { Bad_Opcode },
> + { "notA", { VexGb, Eb }, 0 },
> + { "negA", { VexGb, Eb }, 0 },
> + },
> + /* REG_EVEX_MAP4_F7 */
> + {
> + { Bad_Opcode },
> + { Bad_Opcode },
> + { "notQ", { VexGv, Ev }, 0 },
> + { "negQ", { VexGv, Ev }, 0 },
> + },
> + /* REG_EVEX_MAP4_FE */
> + {
> + { "incA", { VexGb ,Eb }, 0 },
> + { "decA", { VexGb ,Eb }, 0 },
> + },
> + /* REG_EVEX_MAP4_FF */
> + {
> + { "incQ", { VexGv ,Ev }, 0 },
> + { "decQ", { VexGv ,Ev }, 0 },
> + },
Same here, plus for the inc/dec some commas are misplaced. Padding also
looks to be incosnsitent (tab vs blanks).
> --- a/opcodes/i386-dis-evex.h
> +++ b/opcodes/i386-dis-evex.h
>[...]
> @@ -947,23 +947,23 @@ static const struct dis386 evex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> /* 40 */
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> + { "cmovoS", { VexGv, Gv, Ev }, 0 },
> + { "cmovnoS", { VexGv, Gv, Ev }, 0 },
> + { "cmovbS", { VexGv, Gv, Ev }, 0 },
> + { "cmovaeS", { VexGv, Gv, Ev }, 0 },
> + { "cmoveS", { VexGv, Gv, Ev }, 0 },
> + { "cmovneS", { VexGv, Gv, Ev }, 0 },
> + { "cmovbeS", { VexGv, Gv, Ev }, 0 },
> + { "cmovaS", { VexGv, Gv, Ev }, 0 },
> /* 48 */
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> + { "cmovsS", { VexGv, Gv, Ev }, 0 },
> + { "cmovnsS", { VexGv, Gv, Ev }, 0 },
> + { "cmovpS", { VexGv, Gv, Ev }, 0 },
> + { "cmovnpS", { VexGv, Gv, Ev }, 0 },
> + { "cmovlS", { VexGv, Gv, Ev }, 0 },
> + { "cmovgeS", { VexGv, Gv, Ev }, 0 },
> + { "cmovleS", { VexGv, Gv, Ev }, 0 },
> + { "cmovgS", { VexGv, Gv, Ev }, 0 },
Considering CFCMOVcc which sits at the same opcode, doing things like
this sets us up for needing to touch all of these again. Maybe that's
the best that can be done, but I still wonder whether this couldn't
be taken care of right away when introducing these entries.
> @@ -989,7 +989,7 @@ static const struct dis386 evex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> { "wrussK", { M, Gdq }, PREFIX_DATA },
> - { PREFIX_TABLE (PREFIX_EVEX_MAP4_66) },
> + { PREFIX_TABLE (PREFIX_0F38F6) },
Perhaps related to the earlier question: What's going on here?
> @@ -1060,7 +1060,7 @@ static const struct dis386 evex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> { Bad_Opcode },
> - { Bad_Opcode },
> + { "shldS", { VexGv, Ev, Gv, CL }, 0 },
> { Bad_Opcode },
> { Bad_Opcode },
> /* A8 */
> @@ -1069,9 +1069,9 @@ static const struct dis386 evex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> { Bad_Opcode },
> + { "shrdS", { VexGv, Ev, Gv, CL }, 0 },
> { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> + { "imulS", { VexGv, Gv, Ev }, 0 },
PREFIX_OPCODE (or prefix decoding) again missing in all of these?
> @@ -1091,8 +1091,8 @@ static const struct dis386 evex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> /* C0 */
> - { Bad_Opcode },
> - { Bad_Opcode },
> + { REG_TABLE (REG_C0) },
> + { REG_TABLE (REG_C1) },
> { Bad_Opcode },
> { Bad_Opcode },
> { Bad_Opcode },
> @@ -1109,10 +1109,10 @@ static const struct dis386 evex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> /* D0 */
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> + { REG_TABLE (REG_D0) },
> + { REG_TABLE (REG_D1) },
> + { REG_TABLE (REG_D2) },
> + { REG_TABLE (REG_D3) },
Some form of prefix decoding is going to be needed for these too, despite
the goal of wanting to re-use the legacy table entries. Perhaps adding
PREFIX_OPCODE there would be benign to the legacy insns?
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
>[...]
> @@ -2660,47 +2668,47 @@ static const struct dis386 reg_table[][8] = {
> },
> /* REG_D0 */
> {
> - { "rolA", { Eb, I1 }, 0 },
> - { "rorA", { Eb, I1 }, 0 },
> - { "rclA", { Eb, I1 }, 0 },
> - { "rcrA", { Eb, I1 }, 0 },
> - { "shlA", { Eb, I1 }, 0 },
> - { "shrA", { Eb, I1 }, 0 },
> - { "shlA", { Eb, I1 }, 0 },
> - { "sarA", { Eb, I1 }, 0 },
> + { "rolA", { VexGb, Eb, I1 }, 0 },
> + { "rorA", { VexGb, Eb, I1 }, 0 },
> + { "rclA", { VexGb, Eb, I1 }, 0 },
> + { "rcrA", { VexGb, Eb, I1 }, 0 },
> + { "shlA", { VexGb, Eb, I1 }, 0 },
> + { "shrA", { VexGb, Eb, I1 }, 0 },
> + { "shlA", { VexGb, Eb, I1 }, 0 },
> + { "sarA", { VexGb, Eb, I1 }, 0 },
> },
> /* REG_D1 */
> {
> - { "rolQ", { Ev, I1 }, 0 },
> - { "rorQ", { Ev, I1 }, 0 },
> - { "rclQ", { Ev, I1 }, 0 },
> - { "rcrQ", { Ev, I1 }, 0 },
> - { "shlQ", { Ev, I1 }, 0 },
> - { "shrQ", { Ev, I1 }, 0 },
> - { "shlQ", { Ev, I1 }, 0 },
> - { "sarQ", { Ev, I1 }, 0 },
> + { "rolQ", { VexGv, Ev, I1 }, 0 },
> + { "rorQ", { VexGv, Ev, I1 }, 0 },
> + { "rclQ", { VexGv, Ev, I1 }, 0 },
> + { "rcrQ", { VexGv, Ev, I1 }, 0 },
> + { "shlQ", { VexGv, Ev, I1 }, 0 },
> + { "shrQ", { VexGv, Ev, I1 }, 0 },
> + { "shlQ", { VexGv, Ev, I1 }, 0 },
> + { "sarQ", { VexGv, Ev, I1 }, 0 },
> },
As mentioned on the assembler side already, I think we would be better
off making const_1_mode print $1 in AT&T syntax at least for these new
insn forms, to eliminate the ambiguity.
> @@ -9061,6 +9069,15 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
> ins->rex &= ~REX_B;
> ins->rex2 &= ~REX_R;
> }
> + if (ins->evex_type == evex_from_legacy)
> + {
> + /* EVEX from legacy instructions, when the EVEX.ND bit is 0,
> + all bits of EVEX.vvvv and EVEX.V' must be 1. */
> + if (!ins->vex.b && (ins->vex.register_specifier
> + || !ins->vex.v))
> + return &bad_opcode;
> + ins->rex |= REX_OPCODE;
What is this line about?
> @@ -9087,7 +9104,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
> return &err_opcode;
>
> /* Set vector length. */
> - if (ins->modrm.mod == 3 && ins->vex.b)
> + if (ins->modrm.mod == 3 && ins->vex.b && ins->evex_type == evex_default)
> ins->vex.length = 512;
> else
> {
Is this change really needed for anything?
> @@ -9530,6 +9547,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
> {
> oappend (&ins, "{bad}");
> continue;
> +
> }
>
> /* Instructions with a mask register destination allow for
Stray and bogus change.
> @@ -9553,7 +9571,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>
> /* Check whether rounding control was enabled for an insn not
> supporting it. */
> - if (ins.modrm.mod == 3 && ins.vex.b
> + if (ins.modrm.mod == 3 && ins.vex.b && ins.evex_type == evex_default
> && !(ins.evex_used & EVEX_b_used))
> {
This could do with extending the comment, mentioning the aliasing of
EVEX.brs and EVEX.nd.
> @@ -11013,7 +11031,7 @@ print_displacement (instr_info *ins, bfd_signed_vma val)
> static void
> intel_operand_size (instr_info *ins, int bytemode, int sizeflag)
> {
> - if (ins->vex.b)
> + if (ins->vex.b && ins->evex_type == evex_default)
> {
> if (!ins->vex.no_broadcast)
> switch (bytemode)
This aliasing would also be worthwhile mentioning here and ...
> @@ -11946,7 +11964,7 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
> print_operand_value (ins, disp & 0xffff, dis_style_text);
> }
> }
> - if (ins->vex.b)
> + if (ins->vex.b && ins->evex_type == evex_default)
> {
> ins->evex_used |= EVEX_b_used;
... here.
> @@ -13307,6 +13325,14 @@ OP_VEX (instr_info *ins, int bytemode, int sizeflag ATTRIBUTE_UNUSED)
> if (!ins->need_vex)
> return true;
>
> + if (ins->evex_type == evex_from_legacy)
> + {
> + ins->evex_used |= EVEX_b_used;
> + /* Here vex.b is treated as "EVEX.ND. */
Okay, here you have such a helpful comment, just that - nit - there's
an unbalanced double-quote. (The comment would also more logically
come first in this block.)
Jan
next prev parent reply other threads:[~2023-11-08 11:14 UTC|newest]
Thread overview: 113+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-02 11:29 [PATCH v2 0/8] Support Intel APX EGPR Cui, Lili
2023-11-02 11:29 ` [PATCH 1/8] Support APX GPR32 with rex2 prefix Cui, Lili
2023-11-02 17:05 ` Jan Beulich
2023-11-03 6:20 ` Cui, Lili
2023-11-03 13:05 ` Jan Beulich
2023-11-03 14:19 ` Jan Beulich
2023-11-06 15:20 ` Cui, Lili
2023-11-06 16:08 ` Jan Beulich
2023-11-07 8:16 ` Cui, Lili
2023-11-07 10:43 ` Jan Beulich
2023-11-07 15:31 ` Cui, Lili
2023-11-07 15:43 ` Jan Beulich
2023-11-07 15:53 ` Cui, Lili
2023-11-06 15:02 ` Jan Beulich
2023-11-07 8:06 ` Cui, Lili
2023-11-07 10:20 ` Jan Beulich
2023-11-07 14:32 ` Cui, Lili
2023-11-07 15:08 ` Jan Beulich
2023-11-06 15:39 ` Jan Beulich
2023-11-09 8:02 ` Cui, Lili
2023-11-09 10:52 ` Jan Beulich
2023-11-09 13:27 ` Cui, Lili
2023-11-09 15:22 ` Jan Beulich
2023-11-10 7:11 ` Cui, Lili
2023-11-10 9:14 ` Jan Beulich
2023-11-10 9:21 ` Jan Beulich
2023-11-10 12:38 ` Cui, Lili
2023-12-14 10:13 ` Cui, Lili
2023-12-18 15:24 ` Jan Beulich
2023-12-18 16:23 ` H.J. Lu
2023-11-10 9:47 ` Cui, Lili
2023-11-10 9:57 ` Jan Beulich
2023-11-10 12:05 ` Cui, Lili
2023-11-10 12:35 ` Jan Beulich
2023-11-13 0:18 ` Cui, Lili
2023-11-02 11:29 ` [PATCH 2/8] Created an empty EVEX_MAP4_ sub-table for EVEX instructions Cui, Lili
2023-11-02 11:29 ` [PATCH 3/8] Support APX GPR32 with extend evex prefix Cui, Lili
2023-11-02 11:29 ` [PATCH 4/8] Add tests for " Cui, Lili
2023-11-08 9:11 ` Jan Beulich
2023-11-15 14:56 ` Cui, Lili
2023-11-16 9:17 ` Jan Beulich
2023-11-16 15:34 ` Cui, Lili
2023-11-16 16:50 ` Jan Beulich
2023-11-17 12:42 ` Cui, Lili
2023-11-17 14:38 ` Jan Beulich
2023-11-22 13:40 ` Cui, Lili
2023-11-02 11:29 ` [PATCH 5/8] Support APX NDD Cui, Lili
2023-11-08 10:39 ` Jan Beulich
2023-11-20 1:19 ` Cui, Lili
2023-11-08 11:13 ` Jan Beulich [this message]
2023-11-20 12:36 ` Cui, Lili
2023-11-20 16:33 ` Jan Beulich
2023-11-22 7:46 ` Cui, Lili
2023-11-22 8:47 ` Jan Beulich
2023-11-22 10:45 ` Cui, Lili
2023-11-23 10:57 ` Jan Beulich
2023-11-23 12:14 ` Cui, Lili
2023-11-24 6:56 ` [PATCH v3 0/9] Support Intel APX EGPR Cui, Lili
2023-12-07 8:17 ` Cui, Lili
2023-12-07 8:33 ` Cui, Lili
2023-11-09 9:37 ` [PATCH 5/8] Support APX NDD Jan Beulich
2023-11-20 1:33 ` Cui, Lili
2023-11-20 8:19 ` Jan Beulich
2023-11-20 12:54 ` Cui, Lili
2023-11-20 16:43 ` Jan Beulich
2023-11-02 11:29 ` [PATCH 6/8] Support APX Push2/Pop2 Cui, Lili
2023-11-08 11:44 ` Jan Beulich
2023-11-08 12:52 ` Jan Beulich
2023-11-22 5:48 ` Cui, Lili
2023-11-22 8:53 ` Jan Beulich
2023-11-22 12:26 ` Cui, Lili
2023-11-09 9:57 ` Jan Beulich
2023-11-02 11:29 ` [PATCH 7/8] Support APX NDD optimized encoding Cui, Lili
2023-11-09 10:36 ` Jan Beulich
2023-11-10 5:43 ` Hu, Lin1
2023-11-10 9:54 ` Jan Beulich
2023-11-14 2:28 ` Hu, Lin1
2023-11-14 10:50 ` Jan Beulich
2023-11-15 2:52 ` Hu, Lin1
2023-11-15 8:57 ` Jan Beulich
2023-11-15 2:59 ` [PATCH][v3] " Hu, Lin1
2023-11-15 9:34 ` Jan Beulich
2023-11-17 7:24 ` Hu, Lin1
2023-11-17 9:47 ` Jan Beulich
2023-11-20 3:28 ` Hu, Lin1
2023-11-20 8:34 ` Jan Beulich
2023-11-14 2:58 ` [PATCH 1/2] Reorder APX insns in i386.tbl Hu, Lin1
2023-11-14 11:20 ` Jan Beulich
2023-11-15 1:49 ` Hu, Lin1
2023-11-15 8:52 ` Jan Beulich
2023-11-17 3:27 ` Hu, Lin1
2023-11-02 11:29 ` [PATCH 8/8] Support APX JMPABS Cui, Lili
2023-11-09 12:59 ` Jan Beulich
2023-11-14 3:26 ` Hu, Lin1
2023-11-14 11:15 ` Jan Beulich
2023-11-24 5:40 ` Hu, Lin1
2023-11-24 7:21 ` Jan Beulich
2023-11-27 2:16 ` Hu, Lin1
2023-11-27 8:03 ` Jan Beulich
2023-11-27 8:46 ` Hu, Lin1
2023-11-27 8:54 ` Jan Beulich
2023-11-27 9:03 ` Hu, Lin1
2023-11-27 10:32 ` Jan Beulich
2023-12-04 7:33 ` Hu, Lin1
2023-11-02 13:22 ` [PATCH v2 0/8] Support Intel APX EGPR Jan Beulich
2023-11-03 16:42 ` Cui, Lili
2023-11-06 7:30 ` Jan Beulich
2023-11-06 14:20 ` Cui, Lili
2023-11-06 14:44 ` Jan Beulich
2023-11-06 16:03 ` Cui, Lili
2023-11-06 16:10 ` Jan Beulich
2023-11-07 1:53 ` Cui, Lili
2023-11-07 10:11 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f72435bc-d117-8d4e-3391-cc597bf1a89a@suse.com \
--to=jbeulich@suse.com \
--cc=binutils@sourceware.org \
--cc=ccoutant@gmail.com \
--cc=hongjiu.lu@intel.com \
--cc=lili.cui@intel.com \
--cc=lingling.kong@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).