From: Jan Beulich <jbeulich@suse.com>
To: "Cui, Lili" <lili.cui@intel.com>
Cc: hongjiu.lu@intel.com, binutils@sourceware.org
Subject: Re: [PATCH 2/8] Support APX GPR32 with extend evex prefix
Date: Fri, 22 Sep 2023 12:50:35 +0200 [thread overview]
Message-ID: <2a42468b-007a-c501-e2d2-0b5580e8e5b8@suse.com> (raw)
In-Reply-To: <20230919152527.497773-3-lili.cui@intel.com>
On 19.09.2023 17:25, Cui, Lili wrote:
> --- a/opcodes/i386-dis-evex-len.h
> +++ b/opcodes/i386-dis-evex-len.h
> @@ -62,6 +62,16 @@ static const struct dis386 evex_len_table[][3] = {
> { REG_TABLE (REG_EVEX_0F38C7_L_2) },
> },
>
> + /* EVEX_LEN_0F38F2 */
> + {
> + { "andnS", { Gdq, VexGdq, Edq }, 0 },
> + },
> +
> + /* EVEX_LEN_0F38F3 */
> + {
> + { REG_TABLE(REG_EVEX_0F38F3_L_0) },
> + },
> +
> /* EVEX_LEN_0F3A00 */
> {
> { Bad_Opcode },
> diff --git a/opcodes/i386-dis-evex-mod.h b/opcodes/i386-dis-evex-mod.h
> index f9f912c5094..5a1326a1b73 100644
> --- a/opcodes/i386-dis-evex-mod.h
> +++ b/opcodes/i386-dis-evex-mod.h
> @@ -1 +1,51 @@
> /* Nothing at present. */
> + /* MOD_EVEX_MAP4_65 */
> + {
> + { "wrussK", { M, Gdq }, PREFIX_DATA },
> + },
> + /* MOD_EVEX_MAP4_66_PREFIX_0 */
> + {
> + { "wrssK", { M, Gdq }, 0 },
> + },
Not very long ago I invested quite a bit of time to remove unnecessary
decoding through mod_table[]. Please don't introduce new instances.
Entries should be added here only when both branches are populated
(iow it looks as if this patch shouldn't touch this file at all).
> --- a/opcodes/i386-dis-evex-prefix.h
> +++ b/opcodes/i386-dis-evex-prefix.h
> @@ -338,6 +338,89 @@
> { "vcmpp%XH", { MaskG, Vex, EXxh, EXxEVexS, CMP }, 0 },
> { "vcmps%XH", { MaskG, VexScalar, EXw, EXxEVexS, CMP }, 0 },
> },
> + /* PREFIX_EVEX_MAP4_60 */
> + {
> + { "movbeS", { Gv, Ev }, 0 },
> + { Bad_Opcode },
> + { "movbeS", { Gv, Ev }, 0 },
> + },
> + /* PREFIX_EVEX_MAP4_61 */
> + {
> + { "movbeS", { Ev, Gv }, 0 },
> + { Bad_Opcode },
> + { "movbeS", { Ev, Gv }, 0 },
> + },
In cases like this (of which, aiui, there will be many more), where only
prefix 66 is valid, and only to modify operand size, it would be quite
desirable to have a new PREFIX_... identifier to use in the parent table
entry, such that this additional decode step can be avoided.
> --- /dev/null
> +++ b/opcodes/i386-dis-evex-x86.h
I'm puzzled by the name suffix: x86 is kind of redundant with i386. Main
question perhaps is: Do we really need a new file here? It's not a lot
that is put here right now, but of course I haven't peeked ahead.
> --- a/opcodes/i386-dis-evex.h
> +++ b/opcodes/i386-dis-evex.h
> @@ -164,10 +164,10 @@ static const struct dis386 evex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> /* 90 */
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F90) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F91) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F92) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F93) },
> { Bad_Opcode },
> { Bad_Opcode },
> { Bad_Opcode },
> @@ -375,9 +375,9 @@ static const struct dis386 evex_table[][256] = {
> { "vpsllv%DQ", { XM, Vex, EXx }, PREFIX_DATA },
> /* 48 */
> { Bad_Opcode },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F3849) },
> { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F384B) },
> { "vrcp14p%XW", { XM, EXx }, PREFIX_DATA },
> { "vrcp14s%XW", { XMScalar, VexScalar, EXdq }, PREFIX_DATA },
> { "vrsqrt14p%XW", { XM, EXx }, 0 },
> @@ -545,32 +545,32 @@ static const struct dis386 evex_table[][256] = {
> { "%XEvaesdecY", { XM, Vex, EXx }, PREFIX_DATA },
> { "%XEvaesdeclastY", { XM, Vex, EXx }, PREFIX_DATA },
> /* E0 */
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38E0) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38E1) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38E2) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38E3) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38E4) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38E5) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38E6) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38E7) },
> /* E8 */
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38E8) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38E9) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38EA) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38EB) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38EC) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38ED) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38EE) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38EF) },
> /* F0 */
> { Bad_Opcode },
> { Bad_Opcode },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38F2) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38F3) },
> { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> - { Bad_Opcode },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38F5) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38F6) },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F38F7) },
> /* F8 */
> { Bad_Opcode },
> { Bad_Opcode },
> @@ -854,7 +854,7 @@ static const struct dis386 evex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> /* F0 */
> - { Bad_Opcode },
> + { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F3AF0) },
> { Bad_Opcode },
> { Bad_Opcode },
> { Bad_Opcode },
> @@ -872,7 +872,7 @@ static const struct dis386 evex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> },
> - /* EVEX_MAP5_ */
> + /* EVEX_MAP4_ */
While just an artifact from this, ...
> @@ -893,8 +893,8 @@ static const struct dis386 evex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> /* 10 */
> - { PREFIX_TABLE (PREFIX_EVEX_MAP5_10) },
> - { PREFIX_TABLE (PREFIX_EVEX_MAP5_11) },
> + { Bad_Opcode },
> + { Bad_Opcode },
> { Bad_Opcode },
> { Bad_Opcode },
> { Bad_Opcode },
> @@ -907,7 +907,7 @@ static const struct dis386 evex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> { Bad_Opcode },
> - { PREFIX_TABLE (PREFIX_EVEX_MAP5_1D) },
> + { Bad_Opcode },
> { Bad_Opcode },
> { Bad_Opcode },
> /* 20 */
... changes like these are extremely odd to read. Can you please try
to split this patch such that initially you simply introduce an empty
new sub-table, to avoid such anomalies (which will also affect "git
blame" then, I expect)?
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -132,6 +132,13 @@ enum x86_64_isa
> intel64
> };
>
> +enum evex_type
> +{
> + evex_default = 0,
> + evex_from_legacy,
> + evex_from_vex,
> +};
> +
> struct instr_info
> {
> enum address_mode address_mode;
> @@ -212,7 +219,6 @@ struct instr_info
> int ll;
> bool w;
> bool evex;
> - bool r;
The change to eliminate this field would certainly be nice to be separate
from the bulk of thw APX changes here.
Jan
next prev parent reply other threads:[~2023-09-22 10:50 UTC|newest]
Thread overview: 84+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-19 15:25 [PATCH 0/8] [RFC] Support Intel APX EGPR Cui, Lili
2023-09-19 15:25 ` [PATCH 1/8] Support APX GPR32 with rex2 prefix Cui, Lili
2023-09-21 15:27 ` Jan Beulich
2023-09-27 15:57 ` Cui, Lili
2023-09-21 15:51 ` Jan Beulich
2023-09-27 15:59 ` Cui, Lili
2023-09-28 8:02 ` Jan Beulich
2023-10-07 3:27 ` Cui, Lili
2023-09-19 15:25 ` [PATCH 2/8] Support APX GPR32 with extend evex prefix Cui, Lili
2023-09-22 10:12 ` Jan Beulich
2023-10-17 15:48 ` Cui, Lili
2023-10-18 6:40 ` Jan Beulich
2023-10-18 10:44 ` Cui, Lili
2023-10-18 10:50 ` Jan Beulich
2023-09-22 10:50 ` Jan Beulich [this message]
2023-10-17 15:50 ` Cui, Lili
2023-10-17 16:11 ` Jan Beulich
2023-10-18 2:02 ` Cui, Lili
2023-10-18 6:10 ` Jan Beulich
2023-09-25 6:03 ` Jan Beulich
2023-10-17 15:52 ` Cui, Lili
2023-10-17 16:12 ` Jan Beulich
2023-10-18 6:31 ` Cui, Lili
2023-10-18 6:47 ` Jan Beulich
2023-10-18 7:52 ` Cui, Lili
2023-10-18 8:21 ` Jan Beulich
2023-10-18 11:30 ` Cui, Lili
2023-10-19 11:58 ` Cui, Lili
2023-10-19 15:24 ` Jan Beulich
2023-10-19 16:38 ` Cui, Lili
2023-10-20 6:25 ` Jan Beulich
2023-10-22 14:33 ` Cui, Lili
2023-09-19 15:25 ` [PATCH 3/8] Add tests for " Cui, Lili
2023-09-27 13:11 ` Jan Beulich
2023-10-17 15:53 ` FW: " Cui, Lili
2023-10-17 16:19 ` Jan Beulich
2023-10-18 2:32 ` Cui, Lili
2023-10-18 6:05 ` Jan Beulich
2023-10-18 7:16 ` Cui, Lili
2023-10-18 8:05 ` Jan Beulich
2023-10-18 11:26 ` Cui, Lili
2023-10-18 12:06 ` Jan Beulich
2023-10-25 16:03 ` Cui, Lili
2023-09-27 13:19 ` Jan Beulich
2023-09-19 15:25 ` [PATCH 4/8] Support APX NDD Cui, Lili
2023-09-27 14:44 ` Jan Beulich
2023-10-22 14:05 ` Cui, Lili
2023-10-23 7:12 ` Jan Beulich
2023-10-25 8:10 ` Cui, Lili
2023-10-25 8:47 ` Jan Beulich
2023-10-25 15:49 ` Cui, Lili
2023-10-25 15:59 ` Jan Beulich
2023-09-28 7:57 ` Jan Beulich
2023-10-22 14:57 ` Cui, Lili
2023-10-24 11:39 ` Cui, Lili
2023-10-24 11:58 ` Jan Beulich
2023-10-25 15:29 ` Cui, Lili
2023-09-19 15:25 ` [PATCH 5/8] Support APX NDD optimized encoding Cui, Lili
2023-09-28 9:29 ` Jan Beulich
2023-10-23 2:57 ` Hu, Lin1
2023-10-23 7:23 ` Jan Beulich
2023-10-23 7:50 ` Hu, Lin1
2023-10-23 8:15 ` Jan Beulich
2023-10-24 1:40 ` Hu, Lin1
2023-10-24 6:03 ` Jan Beulich
2023-10-24 6:08 ` Hu, Lin1
2023-10-23 3:07 ` [PATCH-V2] " Hu, Lin1
2023-10-23 3:30 ` [PATCH 5/8] [v2] " Hu, Lin1
2023-10-23 7:26 ` Jan Beulich
2023-09-19 15:25 ` [PATCH 6/8] Support APX Push2/Pop2 Cui, Lili
2023-09-28 11:37 ` Jan Beulich
2023-10-30 15:21 ` Cui, Lili
2023-10-30 15:31 ` Jan Beulich
2023-11-20 13:05 ` Cui, Lili
2023-09-19 15:25 ` [PATCH 7/8] Support APX NF Cui, Lili
2023-09-25 6:07 ` Jan Beulich
2023-09-28 12:42 ` Jan Beulich
2023-11-02 10:15 ` Cui, Lili
2023-11-02 10:23 ` Jan Beulich
2023-11-02 10:46 ` Cui, Lili
2023-12-12 2:59 ` H.J. Lu
2023-09-19 15:25 ` [PATCH 8/8] Support APX JMPABS Cui, Lili
2023-09-28 13:11 ` Jan Beulich
2023-11-02 2:32 ` Hu, Lin1
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=2a42468b-007a-c501-e2d2-0b5580e8e5b8@suse.com \
--to=jbeulich@suse.com \
--cc=binutils@sourceware.org \
--cc=hongjiu.lu@intel.com \
--cc=lili.cui@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).