From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x830.google.com (mail-qt1-x830.google.com [IPv6:2607:f8b0:4864:20::830]) by sourceware.org (Postfix) with ESMTPS id F41503858D38 for ; Fri, 14 Oct 2022 21:51:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F41503858D38 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-qt1-x830.google.com with SMTP id h15so4554808qtu.2 for ; Fri, 14 Oct 2022 14:51:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=7eSZuQPa1NaUKDp/8hbZzoeNGWXNF1KutLLvlw2jSME=; b=gNr1XFt7u0fzBexCcNkb7APw0coH7GFr7VK8lYjoEQpLKxpF7UkbyiQlMULojNVFjA 0sm1OF96k0GWrCWg608AbHnMx284L0xNVGVlK5LL6H3CwRC9knS4tpQZUtwfWIQDE83M s0t4PGObAUfQHwhQKwbM21inOOMtRmeigUXgQ+KmNqbdaI87n+80b+2jYsNy1zZ3yZ9Z XVSohRnQHG8I0SAwnPgXZbFXwdBxSdVdbiSuGhZHMTq+6jp1eQifqrgp9oZhB0Msn74A vJt5aR1Qs51DYpct6QIgOSnP5UmZ2XDYNvbLoAeIwEjFC1LEmJLTHFeIPHe58Em+T4be ZDIQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=7eSZuQPa1NaUKDp/8hbZzoeNGWXNF1KutLLvlw2jSME=; b=N2uUjPUAIgUtHyGbL6766tte0TgPRg14h3aDNqOGo55b5bpwvDlxgVwLAGJ3ZHSrSl GSWqpnjX4IiKDWHya8Cv/6lu1NptRhQ7DMm33y8EKC3SJGJicVkXHuc6AoNMyBA6Q9/m amd+iZgl1YMaEuTAUSgR4aHp5mcYCSKty1O57koOz3+22tt5FbNfvolpLpCmLRC7lbW7 mvkjF4tcOi7bdk5/JWfASaiP/GLs+bx6hUjcfOQ3SnuU94H5EkD0I6oV0bR2qAONapzf VMq55p2/ZN5oasBuVmYVdqaXUQz2bEyU9kC4cfnt4b345ZVxSSi5vIn11YGagQHxQeBg 8xiw== X-Gm-Message-State: ACrzQf0l9b5BbIjsffdQAXLMqhiMUSPAtgCc6UA8urr8xlCjUPv7Nprn Gl++IYqQYFRABWXRKETYqF38Vn1SxKLCspS9ySfuFJhp X-Google-Smtp-Source: AMsMyM7GRToNPg6I7JmEvsQLjl3rlPTw2WKu0xwNVDvQOr8rbTA5BDnz++fsuWx1cGJjdvcaIuPXXm7KjTeRFf0cQtA= X-Received: by 2002:ac8:4e51:0:b0:395:4f2c:63c9 with SMTP id e17-20020ac84e51000000b003954f2c63c9mr5858064qtw.617.1665784313330; Fri, 14 Oct 2022 14:51:53 -0700 (PDT) MIME-Version: 1.0 References: <20221014091248.4920-1-haochen.jiang@intel.com> <20221014091248.4920-5-haochen.jiang@intel.com> <1d847a52-b1ff-b816-1507-7077724901bb@suse.com> In-Reply-To: From: "H.J. Lu" Date: Fri, 14 Oct 2022 14:51:17 -0700 Message-ID: Subject: Re: [PATCH 04/10] Support Intel CMPccXADD To: Jan Beulich Cc: Haochen Jiang , binutils@sourceware.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3018.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham 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 Fri, Oct 14, 2022 at 11:27 AM H.J. Lu wrote: > > On Fri, Oct 14, 2022 at 6:46 AM Jan Beulich wrote: > > > > On 14.10.2022 11:12, Haochen Jiang wrote: > > > --- a/gas/NEWS > > > +++ b/gas/NEWS > > > @@ -1,5 +1,7 @@ > > > -*- text -*- > > > > > > +* Add support for Intel CMPccXADD instructions. > > > + > > > * Add support for Intel AVX-NE-CONVERT instructions. > > > > > > * Add support for Intel AVX-VNNI-INT8 instructions. > > > > I wonder if all of these really need a separate line. > > > > > --- a/gas/config/tc-i386.c > > > +++ b/gas/config/tc-i386.c > > > @@ -1097,6 +1097,7 @@ static const arch_entry cpu_arch[] = > > > SUBARCH (avx_ifma, AVX_IFMA, ANY_AVX_IFMA, false), > > > SUBARCH (avx_vnni_int8, AVX_VNNI_INT8, ANY_AVX_VNNI_INT8, false), > > > SUBARCH (avx_ne_convert, AVX_NE_CONVERT, ANY_AVX_NE_CONVERT, false), > > > + SUBARCH (cmpccxadd, CMPCCXADD, ANY_CMPCCXADD, false) > > > }; > > > > No need for ANY_CMPCCXADD, unless you _know_ dependent features will appear. > > See e.g. FSGSBASE, i.e. you can use CMPCCXADD twice here. > > > > > --- a/opcodes/i386-dis.c > > > +++ b/opcodes/i386-dis.c > > > @@ -366,6 +366,7 @@ fetch_data (struct disassemble_info *info, bfd_byte *addr) > > > #define Ma { OP_M, a_mode } > > > #define Mb { OP_M, b_mode } > > > #define Md { OP_M, d_mode } > > > +#define Mdq { OP_M, dq_mode } > > > > You're decoding via mod_table[], so I don't think you need this. Or (perhaps > > better) vice versa - keep this (if there's no pre-existing one that fits) and > > avoid the decode step through mod_table[]. > > > > > @@ -939,6 +940,22 @@ enum > > > MOD_VEX_0F388E, > > > MOD_VEX_0F38B0, > > > MOD_VEX_0F38B1, > > > + MOD_VEX_0F38E0_X86_64, > > > + MOD_VEX_0F38E1_X86_64, > > > + MOD_VEX_0F38E2_X86_64, > > > + MOD_VEX_0F38E3_X86_64, > > > + MOD_VEX_0F38E4_X86_64, > > > + MOD_VEX_0F38E5_X86_64, > > > + MOD_VEX_0F38E6_X86_64, > > > + MOD_VEX_0F38E7_X86_64, > > > + MOD_VEX_0F38E8_X86_64, > > > + MOD_VEX_0F38E9_X86_64, > > > + MOD_VEX_0F38EA_X86_64, > > > + MOD_VEX_0F38EB_X86_64, > > > + MOD_VEX_0F38EC_X86_64, > > > + MOD_VEX_0F38ED_X86_64, > > > + MOD_VEX_0F38EE_X86_64, > > > + MOD_VEX_0F38EF_X86_64, > > > > Hmm, I really need to split off (and re-submit) the re-usable parts of > > "x86-64: Intel64 adjustments for conditional jumps" (see > > https://sourceware.org/pipermail/binutils/2020-July/112365.html), to avoid > > the need for 16 almost identical entries of several kinds throughout this > > patch. > > > > > @@ -8480,6 +8609,70 @@ static const struct dis386 mod_table[][2] = { > > > /* MOD_VEX_0F38B1*/ > > > { VEX_W_TABLE (VEX_W_0F38B1) }, > > > }, > > > + { > > > + /* MOD_VEX_0F38E0_X86_64 */ > > > + { "cmpoxadd", { Mdq, Gdq, VexGdq }, PREFIX_DATA }, > > > + }, > > > + { > > > + /* MOD_VEX_0F38E1_X86_64 */ > > > + { "cmpnoxadd", { Mdq, Gdq, VexGdq }, PREFIX_DATA }, > > > + }, > > > + { > > > + /* MOD_VEX_0F38E2_X86_64 */ > > > + { "cmpbxadd", { Mdq, Gdq, VexGdq }, PREFIX_DATA }, > > > + }, > > > + { > > > + /* MOD_VEX_0F38E3_X86_64 */ > > > + { "cmpnbxadd", { Mdq, Gdq, VexGdq }, PREFIX_DATA }, > > > > I understand the ISA extensions document names the insn this way and doesn't > > list cmpaexadd (same for other aliases), but I think this is a mistake in > > Lack of aliases is a bad thing. In any case, assembler should follow Oops. I meant "Lack of aliases isn't a bad thing." Aliases make me wonder if 2 different jcc are really different. > the spec. > > > the doc. I've raised a respective question in the ISA extensions forum: I > > think representation of conditions to check for should be uniform among > > insns, and hence it should be "ae" here. (That would also be the effect if > > you used %C here.) > > > > > @@ -433,7 +436,7 @@ typedef union i386_cpu_flags > > > unsigned int cpu64:1; > > > unsigned int cpuno64:1; > > > #ifdef CpuUnused > > > - unsigned int unused:(CpuNumOfBits - CpuUnused); > > > + // unsigned int unused:(CpuNumOfBits - CpuUnused); > > > #endif > > > > No - you should instead comment out the #define of CpuUnused - see the > > comment there. > > > > > --- a/opcodes/i386-opc.tbl > > > +++ b/opcodes/i386-opc.tbl > > > @@ -3296,3 +3296,24 @@ vpdpbsud, 0xf350, None, CpuAVX_VNNI_INT8, Modrm|Vex|Space0F38|VexVVVV|VexW0|Chec > > > vpdpbsuds, 0xf351, None, CpuAVX_VNNI_INT8, Modrm|Vex|Space0F38|VexVVVV|VexW0|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { RegXMM|RegYMM|Unspecified|BaseIndex, RegXMM|RegYMM, RegXMM|RegYMM } > > > > > > // AVX_VNNI_INT8 instructions end. > > > + > > > +// CMPCCXADD instructions. > > > + > > > +cmpbexadd, 0x66e6, None, CpuCMPCCXADD|Cpu64, Modrm|Vex128|Space0F38|VexVVVV=1|SwapSources|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Reg32|Reg64, Reg32|Reg64, Dword|Qword|Unspecified|BaseIndex } > > > > Along the lines of the earlier comment - you want to use the > > template here, eliminating the need for 16 almost identical lines _and_ > > supplying all condition code representation in one go. > > > > Apart from that you forgot CheckRegSize here afaict. And please again > > VexVVVV alone, without =1. Also for non-vector insns perhaps better plain > > Vex instead of Vex128. Further these insns should allow for l and q > > suffixes in AT&T mode. > > l and q suffixes here are totally unnecessary. For new instructions, > suffixes should be required only if needed. > > > And finally - is SwapSources really appropriate to use here? There's only > > one pure source operand, the other two are also serving as destinations. > > I wonder whether an attribute is necessary here in the first place: Vex- > > encoded insns with a memory destination never have two further register > > operands, so that property should suffice for identifying the case in > > build_modrm_byte(). Alternatively you could also simply use the CPU flag. > > > > Jan > > > > -- > H.J. -- H.J.