From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com [IPv6:2a00:1450:4864:20::62f]) by sourceware.org (Postfix) with ESMTPS id B139F385DC01 for ; Fri, 8 Mar 2024 09:36:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B139F385DC01 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org B139F385DC01 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::62f ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709890584; cv=none; b=ASIEe8Afi8I1iH+AuhFI6hlFVAM3Ht3ztTAExAZBO6oloEBBlMCsenHjDygyE8CyS4jxyV5cNLGaL2IuhZDY5uryRw+MHNo1ff8oeO/Um3LMOz0riAhDE7QX5qidg5d5IKtlO8XCjaDbzUsxaO6oW4uZfDdt06VSD1OB0wZAISA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709890584; c=relaxed/simple; bh=UzCiugNJUWUJLwLJ+V3yvk5p5kOuyW3hOk4MH/pSG04=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=AnPqJowGG95xAmNDG+VQLwAXqlGT/zfzsaqcKhLWbtV1L8p5CfLnODX/pEUwQjCuJ6NnERbewZBtJFFF7s/IDZXJeV1pyMJEF8yXK0jnlvgFe30WecyOhwHBamhN6ChAaJrarksCjwqwwur2S/AEGM5rKUkmQfw/L/j9b27L5sg= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-ej1-x62f.google.com with SMTP id a640c23a62f3a-a44d084bfe1so76699566b.1 for ; Fri, 08 Mar 2024 01:36:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1709890580; x=1710495380; darn=sourceware.org; h=content-transfer-encoding:in-reply-to:autocrypt:from:references:cc :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=RceDDE1X8sHMdycrPILjpI0XSX7eu3DJdpM56b8pMUI=; b=Nayu3iPJSqO1PLzg6OaZfsI+q/fPjSksDTpgfmm2ZXUjV/WNVyri3GX1WbbBGcvLBo EXl8QO7bHCpZW/YqhVwSPLRp5fZqYFZ/wJ81X+OYxIQAs774WNEU8iE7RoB2r7rxFZA4 rBHG+onZYFwnOMYm8J/SFdaeSrhAeafs6ZsBEhkYjaEyfAsr02TuevWO4mTFqUlga8w4 a3k3i+iVYZnYwENUmVW0k63gWzmOZeLUICCnui/db+iS7XyBVLafZs2XfW2Puv+thGa+ 36hhnVIrbtL0HuETyM5YTyfzLwa6uSgkwt6Gvd2dvlpVWlgFClRHMcmkTEUvL1J1bZle REsQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709890580; x=1710495380; h=content-transfer-encoding:in-reply-to:autocrypt:from:references:cc :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=RceDDE1X8sHMdycrPILjpI0XSX7eu3DJdpM56b8pMUI=; b=fLNNH31sZud054PySokUcMjhbgcANpytQDKzw9rzeu2L/B/NuLqI77enXl+UAPCIi0 yP4mdb3oR9I11V4p1qeSb53vWLCmnKI8rbrFeOJcIZ7RZPaWuupakKjdX3vVVT15KeEc d7Wb1NO5EDBVwxShTP657QW5SyXDP/XbeVnc402RWBu9Yugb6vlehVIXYTcpkK9D7so2 9yRJ4fVJRvM6wrO0w8e1jg4C6EQHOJ8bzp3cpw7ceu2FHJbRsZ8ouEFZxReYVQNf4A/q rCN7oocPrO4z6Q6iOJt3x+x6PvB/2ugj7HK3wzqE3aL/da9M78co/Qop6SkNGiiupZre nVWA== X-Forwarded-Encrypted: i=1; AJvYcCWJznMLJ9Kf4SJCFjYFpWSi1JIpgfcuD4FRB5Id+EPiYLaa46XqmD82qRZxfUd6afz0BpVn3P6SjgSDG2TkK1QeDurMV6d8GQ== X-Gm-Message-State: AOJu0Yws63WrR+SSx5V7QFjctn1IR270f1FkDE+EUixl2ab7NaW/KQyK eGw9UShpCZcxxMvxUYfCpd8gbaqgn1iF8z73TmuSPSrWc7BsWGXSOuB9rBIcqw== X-Google-Smtp-Source: AGHT+IFfRkJPhSp1lcl5v1tMnnOpoQb0EPTpCk3NfPRYz24SMuNDfzvw571X9IOSr2c2yBN7uwdjGQ== X-Received: by 2002:a17:906:cf88:b0:a45:373:cff with SMTP id um8-20020a170906cf8800b00a4503730cffmr11290080ejb.68.1709890580166; Fri, 08 Mar 2024 01:36:20 -0800 (PST) Received: from [10.156.60.236] (ip-037-024-206-209.um08.pools.vodafone-ip.de. [37.24.206.209]) by smtp.gmail.com with ESMTPSA id m19-20020a17090607d300b00a45cabd9b5asm1720916ejc.111.2024.03.08.01.36.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 08 Mar 2024 01:36:19 -0800 (PST) Message-ID: <84c732bb-ec84-416b-bc69-29f4f3b9ad0e@suse.com> Date: Fri, 8 Mar 2024 10:36:18 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH V2] Support APX NF Content-Language: en-US To: "Cui, Lili" Cc: hjl.tools@gmail.com, binutils@sourceware.org References: <20240304081527.1724628-1-lili.cui@intel.com> From: Jan Beulich Autocrypt: addr=jbeulich@suse.com; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL In-Reply-To: <20240304081527.1724628-1-lili.cui@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3022.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,LOTS_OF_MONEY,MONEY_NOHTML,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 04.03.2024 09:15, Cui, Lili wrote: > gas/ChangeLog: > > * NEWS: Support Intel APX NF. > * config/tc-i386.c (enum i386_error): Add unsupported_nf. > (struct _i386_insn): Add has_nf. > (is_apx_evex_encoding): Ditto. > (build_apx_evex_prefix): Encode the NF bit. > (md_assemble): Handle unsupported_nf. > (parse_insn): Handle Prefix_NF. > (can_convert_NDD_to_legacy): > (match_template): Support D for APX_F insns. > * testsuite/gas/i386/x86-64-apx-evex-promoted-bad.d: Add bad test for NF bit. > * testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s: Ditto. > * testsuite/gas/i386/x86-64-apx-inval.l: Ditto. > * testsuite/gas/i386/x86-64-apx-inval.s: Ditto. > * testsuite/gas/i386/x86-64.exp: Add apx nf tests. > * testsuite/gas/i386/x86-64-apx-nf-intel.d: New test. > * testsuite/gas/i386/x86-64-apx-nf.d: Ditto. > * testsuite/gas/i386/x86-64-apx-nf.s: Ditto. > > opcodes/ChangeLog: > > * i386-dis-evex.h: Add %XN to the instructions that support APX NF and > add new instruction imul, popcnt, tzcnt and lzcnt to EVEX table. > * i386-dis-evex-reg.h: Ditto. > * i386-dis.c (struct instr_info): Add nf. > (struct dis386): Add "NF" for EVEX.NF. > (get_valid_dis386): Set ins->vex.nf. > (print_insn): Handle ins.vex.nf. > (putop): Handle "%NF". > * i386-opc.h (Prefix_NoOptimize): Adjust the value. Stale entry? As asked for before: If you provide this information, it needs to be adjusted as patch contents actually changes. > (Prefix_NF): New. > * i386-opc.tbl: Add new entries for the instructions that support APX NF. ... plus a few that don't (while others that don't and aren't of use except when used with {evex} were added prematurely). > @@ -6655,6 +6663,9 @@ md_assemble (char *line) > case unsupported_EGPR_for_addressing: > err_msg = _("extended GPR cannot be used as base/index"); > break; > + case unsupported_nf: > + err_msg = _("unsupported NF"); > + break; "{nf} unsupported" (yielding an overall better complete diagnostic)? > @@ -7218,6 +7229,11 @@ parse_insn (const char *line, char *mnemonic, bool prefix_only) > /* {rex2} */ > i.rex2_encoding = true; > break; > + case Prefix_NF: > + /* {nf} */ > + i.has_nf = true; > + i.encoding = encoding_evex; > + break; It's not quite as easy, I'm afraid: Have you thought of the "{vex} {nf} ..." case? (I think I previously indicated that their combination, actually in either order, needs properly rejecting.) Without having spent much thought on it, perhaps it would suffice to check here that the field is still encoding_default, and leave the value alone otherwise (in order to reject bad combinations elsewhere). > --- a/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s > +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s > @@ -23,7 +23,7 @@ _start: > .insn EVEX.L1.66.M12.W0 0x60, %di, %ax > > #EVEX_MAP4 movbe %r18w,%ax set EVEX.z == 0b1. > - .insn EVEX.L0.66.M12.W0 0x60, %di, %ax {%k7}{z} > + .insn EVEX.L0.66.M12.W0 0x60, %di, %ax {%k3}{z} What's going on here ... > @@ -33,17 +33,25 @@ _start: > .insn EVEX.L1.NP.0f38.W1 0xf5, %rax, (%rax,%rbx), %rcx > > #EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[23](EVEX.z) == 0b1 > - .insn EVEX.L0.NP.0f38.W1 0xf5, %rax, (%rax,%rbx), %rcx {%k7}{z} > + .insn EVEX.L0.NP.0f38.W1 0xf5, %rax, (%rax,%rbx), %rcx {%k3}{z} ... and here? This is the kind of thing that wants mentioning in the patch description. And once again, especially for larger patches it is generally a bad idea if they come with only legacy ChangeLog entries: These only say "what", never "why". (That said, I think I see why the change is needed.) Additionally may I ask that you get used to providing a brief revision log when sending new versions of earlier patches? This helps reviewers when they did look at the earlier version(s). > #EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[20](EVEX.b) == 0b1 > .insn EVEX.L0.NP.0f38.W1 0xf5, %rax, (%rax,%rbx){1to8}, %rcx > > #{evex} inc %rax %rbx EVEX.vvvv != 1111 && EVEX.ND = 0. > .byte 0x62, 0xf4, 0xe4, 0x08, 0xff, 0x04, 0x08 > + > # pop2 %rax, %r8 set EVEX.ND=0. > .byte 0x62, 0xf4, 0x3c, 0x08, 0x8f, 0xc0 > .byte 0xff, 0xff, 0xff > + > # pop2 %rax, %r8 set EVEX.vvvv = 1111. > .insn EVEX.L0.M4.W0 0x8f, %rax, {rn-sae},%r8 > + > # pop2 %r8, %r8. > .byte 0x62, 0xd4, 0x3c, 0x18, 0x8f, 0xc0 > + .byte 0xff, 0xff As indicated - this would better be avoided by picking a more suitable ModR/M byte on the earlier line. Using %r11 instead of %r8 ought to do just fine here, I suppose. > + #EVEX_MAP4 movbe %r18w,%ax set EVEX.nf = 1. > + .insn EVEX.L0.66.M12.W0 0x60, %di, %ax {%k4} Nit: Inconsistent indentation. > --- /dev/null > +++ b/gas/testsuite/gas/i386/x86-64-apx-nf.s >[...] > + {nf} rorq %cl, 291(%r8, %rax, 4) > + {nf} ror %cl, 291(%r8, %rax, 4), %r9 > + {nf} sar $1, %bl > + {nf} sar $1, %bl, %dl > + {nf} sar $1, %dx > + {nf} sar $1, %dx, %ax > + {nf} sar $1, %ecx > + {nf} sar $1, %ecx, %edx > + {nf} sar $1, %r9 > + {nf} sar $1, %r9, %r31 > + {nf} sarb $1, 291(%r8, %rax, 4) > + {nf} sar $1, 291(%r8, %rax, 4), %bl > + {nf} sarw $1, 291(%r8, %rax, 4) > + {nf} sar $1, 291(%r8, %rax, 4), %dx > + {nf} sarl $1, 291(%r8, %rax, 4) > + {nf} sar $1, 291(%r8, %rax, 4), %ecx > + {nf} sarq $1, 291(%r8, %rax, 4) > + {nf} sar $1, 291(%r8, %rax, 4), %r9 > + {nf} sar $123, %bl > + {nf} sar $123, %bl, %dl > + {nf} sar $123, %dx > + {nf} sar $123, %dx, %ax > + {nf} sar $123, %ecx > + {nf} sar $123, %ecx, %edx > + {nf} sar $123, %r9 > + {nf} sar $123, %r9, %r31 > + {nf} sarb $123, 291(%r8, %rax, 4) > + {nf} sar $123, 291(%r8, %rax, 4), %bl > + {nf} sarw $123, 291(%r8, %rax, 4) > + {nf} sar $123, 291(%r8, %rax, 4), %dx > + {nf} sarl $123, 291(%r8, %rax, 4) > + {nf} sar $123, 291(%r8, %rax, 4), %ecx > + {nf} sarq $123, 291(%r8, %rax, 4) > + {nf} sar $123, 291(%r8, %rax, 4), %r9 > + {nf} sar %cl, %bl > + {nf} sar %cl, %bl, %dl > + {nf} sar %cl, %dx > + {nf} sar %cl, %dx, %ax > + {nf} sar %cl, %ecx > + {nf} sar %cl, %ecx, %edx > + {nf} sar %cl, %r9 > + {nf} sar %cl, %r9, %r31 > + {nf} sarb %cl, 291(%r8, %rax, 4) > + {nf} sar %cl, 291(%r8, %rax, 4), %bl > + {nf} sarw %cl, 291(%r8, %rax, 4) > + {nf} sar %cl, 291(%r8, %rax, 4), %dx > + {nf} sarl %cl, 291(%r8, %rax, 4) > + {nf} sar %cl, 291(%r8, %rax, 4), %ecx > + {nf} sarq %cl, 291(%r8, %rax, 4) > + {nf} sar %cl, 291(%r8, %rax, 4), %r9 > + {nf} sal $1, %bl > + {nf} sal $1, %bl, %dl Hmm "sal" sorts ahead of "sar", doesn't it? > --- a/opcodes/i386-dis-evex-reg.h > +++ b/opcodes/i386-dis-evex-reg.h > @@ -51,33 +51,33 @@ > }, > /* REG_EVEX_MAP4_80 */ > { > - { "addA", { VexGb, Eb, Ib }, NO_PREFIX }, > - { "orA", { VexGb, Eb, Ib }, NO_PREFIX }, > + { "%NFaddA", { VexGb, Eb, Ib }, NO_PREFIX }, > + { "%NForA", { VexGb, Eb, Ib }, NO_PREFIX }, > { "adcA", { VexGb, Eb, Ib }, NO_PREFIX }, > { "sbbA", { VexGb, Eb, Ib }, NO_PREFIX }, You (correctly) leave these alone. > @@ -87,25 +87,33 @@ > { > { Bad_Opcode }, > { Bad_Opcode }, > - { "notA", { VexGb, Eb }, NO_PREFIX }, > - { "negA", { VexGb, Eb }, NO_PREFIX }, > + { "%NFnotA", { VexGb, Eb }, NO_PREFIX }, Why does this gain %NF then? > @@ -1815,6 +1817,8 @@ struct dis386 { > "XV" => print "{vex} " pseudo prefix > "XE" => print "{evex} " pseudo prefix if no EVEX-specific functionality is > is used by an EVEX-encoded (AVX512VL) instruction. > + "NF" => print "{nf} " pseudo prefix when EVEX.NF = 1. > + a valid encoding. What is this 2nd line, not even a complete sentence, supposed to be saying? > @@ -2619,25 +2623,25 @@ static const struct dis386 reg_table[][8] = { > }, > /* REG_C0 */ > { > - { "rolA", { VexGb, Eb, Ib }, NO_PREFIX }, > - { "rorA", { VexGb, Eb, Ib }, NO_PREFIX }, > - { "rclA", { VexGb, Eb, Ib }, NO_PREFIX }, > - { "rcrA", { VexGb, Eb, Ib }, NO_PREFIX }, > - { "shlA", { VexGb, Eb, Ib }, NO_PREFIX }, > - { "shrA", { VexGb, Eb, Ib }, NO_PREFIX }, > - { "shlA", { VexGb, Eb, Ib }, NO_PREFIX }, > - { "sarA", { VexGb, Eb, Ib }, NO_PREFIX }, > + { "%NFrolA", { VexGb, Eb, Ib }, NO_PREFIX }, > + { "%NFrorA", { VexGb, Eb, Ib }, NO_PREFIX }, > + { "%NFrclA", { VexGb, Eb, Ib }, NO_PREFIX }, > + { "%NFrcrA", { VexGb, Eb, Ib }, NO_PREFIX }, Like NOT, RC{L,R} look to be wrongly being modified here. Interestingly for D0...D3 you leave them alone. > @@ -9147,6 +9151,10 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins) > ins->vex.v = *ins->codep & 0x8; > ins->vex.mask_register_specifier = *ins->codep & 0x7; > ins->vex.zeroing = *ins->codep & 0x80; > + /* Set the NF bit for the EVEX instruction extended from the legacy or > + vex instruction, this bit will be cleared when it can be confirmed > + that its defaut type is evex. */ > + ins->vex.nf = *ins->codep & 0x4; Whow or what is "its" in the comment? Also nit: "default". Assuming it's the insn that's meant, how about "when it's an evex_default one"? > @@ -9600,10 +9608,21 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax) > && ins.vex.prefix == DATA_PREFIX_OPCODE) > sizeflag ^= DFLAG; > > + if(ins.evex_type == evex_default) > + ins.vex.nf = false; > + else > + /* For EVEX-promoted formats, we need to clear EVEX.NF (For ccmp and > + ctest, they will be cleared separately.) in mask_register_specifier > + and keep the low 2 bits of mask_register_specifier to report errors > + for invalid cases.*/ > + ins.vex.mask_register_specifier &= 0x3; > + > if (dp != NULL && putop (&ins, dp->name, sizeflag) == 0) > { > if (!get_sib (&ins, sizeflag)) > goto fetch_error_out; > + if (!ins.has_nf && ins.vex.nf) > + oappend (&ins, "{bad}"); As can be seen from the testcase additions, this suffixes onto the insn menmonic. Since it's a bogus (pseudo) prefix, maybe better also print as such, and then maybe also indicate what it is that is bad (e.g. {bad-nf})? > @@ -10564,6 +10583,29 @@ putop (instr_info *ins, const char *in_template, int sizeflag) > } > else if (l == 1 && last[0] == 'C') > break; > + else if (l == 1 && last[0] == 'N') > + { > + ins->has_nf = true; > + > + if (ins->vex.nf == true) > + { > + *ins->obufp++ = '{'; > + *ins->obufp++ = 'n'; > + *ins->obufp++ = 'f'; > + *ins->obufp++ = '}'; > + *ins->obufp++ = ' '; > + } Do we really need two booleans here? Can't you simply clear ->vex.nf here, making print_insn() spot when it's still set? > + else if (ins->evex_type == evex_from_legacy && !ins->vex.b) > + { > + *ins->obufp++ = '{'; > + *ins->obufp++ = 'e'; > + *ins->obufp++ = 'v'; > + *ins->obufp++ = 'e'; > + *ins->obufp++ = 'x'; > + *ins->obufp++ = '}'; > + *ins->obufp++ = ' '; > + } This looks unrelated (by the title perhaps more related to the other patch you sent subsequently), and is also not tested anywhere if I'm not mistaken. I don't mind it being kept here, but besides needing at least minimal testing this is then also something that wants mentioning in the description (for going beyond the basic purpose of the patch). Plus this effect also needs mentioning above in the documentation of "NF". Finally for both parts: Why not oappend() or even oappend_with_style()? I realize we have other similar pieces of code, but I question their correctness the latest after styling was added. > --- a/opcodes/i386-opc.tbl > +++ b/opcodes/i386-opc.tbl > @@ -310,32 +310,42 @@ sti, 0xfb, 0, NoSuf, {} > // Arithmetic. > add, 0x0, APX_F, D|C|W|CheckOperandSize|Modrm|No_sSuf|DstVVVV|EVexMap4|NF, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg8|Reg16|Reg32|Reg64 } > add, 0x0, 0, D|W|CheckOperandSize|Modrm|No_sSuf|HLEPrefixLock, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } > +add, 0x0, APX_F, D|W|CheckOperandSize|Modrm|No_sSuf|EVexMap4|NF, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } > add, 0x83/0, APX_F, Modrm|CheckOperandSize|No_bSuf|No_sSuf|DstVVVV|EVexMap4|NF, { Imm8S, Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 } > add, 0x83/0, 0, Modrm|No_bSuf|No_sSuf|HLEPrefixLock, { Imm8S, Reg16|Reg32|Reg64|Unspecified|BaseIndex } > +add, 0x83/0, APX_F, Modrm|No_bSuf|No_sSuf|EVexMap4|NF, { Imm8S, Reg16|Reg32|Reg64|Unspecified|BaseIndex } > add, 0x4, 0, W|No_sSuf, { Imm8|Imm16|Imm32|Imm32S, Acc|Byte|Word|Dword|Qword } > add, 0x80/0, APX_F, W|Modrm|CheckOperandSize|No_sSuf|DstVVVV|EVexMap4|NF, { Imm8|Imm16|Imm32|Imm32S, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg8|Reg16|Reg32|Reg64} > add, 0x80/0, 0, W|Modrm|No_sSuf|HLEPrefixLock, { Imm8|Imm16|Imm32|Imm32S, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } > +add, 0x80/0, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, { Imm8|Imm16|Imm32|Imm32S, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } As before, this (and similar new ones) can also be picked up without use of {nf}, which will want testing. If you deliberately defer adding such a test, this needs saying in the description. One further remark: In the course of adding APX support the set of templates for ADD and its sibling ALU ops was bumped from 4 to 10. There was quite a bit of redundancy between these 7 groups before, but it's growing much worse now. Did you consider templatizing them up front? This may even be worthwhile for just the ... > inc, 0x40, No64, No_bSuf|No_sSuf|No_qSuf, { Reg16|Reg32 } > inc, 0xfe/0, APX_F, W|Modrm|No_sSuf|CheckOperandSize|DstVVVV|EVexMap4|NF, {Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg8|Reg16|Reg32|Reg64} > inc, 0xfe/0, 0, W|Modrm|No_sSuf|HLEPrefixLock, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } > +inc, 0xfe/0, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } ... pair of INC/DEC as well, despite it their number only going from 2 to 4. And quite certainly also for others below. > @@ -434,24 +464,34 @@ imul, 0x69, i186, Modrm|No_bSuf|No_sSuf|RegKludge, { Imm16|Imm32|Imm32S, Reg16|R > > div, 0xf6/6, 0, W|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } > div, 0xf6/6, 0, W|CheckOperandSize|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, Acc|Byte|Word|Dword|Qword } > +div, 0xf6/6, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } > idiv, 0xf6/7, 0, W|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } > idiv, 0xf6/7, 0, W|CheckOperandSize|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, Acc|Byte|Word|Dword|Qword } > +idiv, 0xf6/7, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } What about the secondary forms with an explicit destination operand? > @@ -2027,6 +2090,7 @@ blsi, 0xf3/3, APX_F(BMI), Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVV > blsmsk, 0xf3/2, APX_F(BMI), Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVVV|No_bSuf|No_wSuf|No_sSuf|NF, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 } > blsr, 0xf3/1, APX_F(BMI), Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVVV|No_bSuf|No_wSuf|No_sSuf|NF, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 } > tzcnt, 0xf30fbc, BMI, Modrm|CheckOperandSize|No_bSuf|No_sSuf, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 } > +tzcnt, 0xf4, BMI&APX_F, Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4|NF, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 } Like e.g. here, ... > @@ -2104,9 +2168,11 @@ insertq, 0xf20f78, SSE4a, Modrm|NoSuf, { Imm8, Imm8, RegXMM, RegXMM } > > // LZCNT instruction > lzcnt, 0xf30fbd, LZCNT, Modrm|CheckOperandSize|No_bSuf|No_sSuf, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 } > +lzcnt, 0xf5, LZCNT|APX_F, Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4|NF, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 } ... I think you mean LZCNT&APX_F here. > // POPCNT instruction > popcnt, 0xf30fb8, POPCNT, Modrm|CheckOperandSize|No_bSuf|No_sSuf, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 } > +popcnt, 0x88, POPCNT|APX_F, Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4|NF, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 } Whereas here it ought to be just APX_F if the doc is to be trusted. Jan