From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com [IPv6:2a00:1450:4864:20::42e]) by sourceware.org (Postfix) with ESMTPS id CA52F3858C98 for ; Mon, 11 Dec 2023 11:17:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CA52F3858C98 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 CA52F3858C98 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::42e ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702293482; cv=none; b=JT+Toa0NisjsyE/LH9k24d6kaZnhI5G2Fw5jpJe5RNoFAfzsReoYK/SgZBFer+9KxLa9GQgz9Tni7iMeIAoGuzW6nQSrNumwfPVLbyWN/pSTJSQaddZuIKwBf+fzQ/bH9Av/0m7XzfYR5LHYWYYBcddCYZx7q7OMnx+hqliUudg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702293482; c=relaxed/simple; bh=Riq2i41WHCXJ6Vk18oyEdvXg//0PaNStkQlS2LyDiF8=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=sE1tVZ96FBRJb5pm0XtVAVUa1v6PwjLqTlqNUxw4PY0UghPTiO1SFu9Yj1Y3FXSKzZsD8yN2wxIErXeSg/VPPDyVGmujcCuHks3GiSXVA+ovASsGd9Oa+z45z1FuaXKMpPlf1yteGg8r1QbP2uYrmEwfRQvnPVqRvSCsQf/UbiI= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wr1-x42e.google.com with SMTP id ffacd0b85a97d-336221efdceso578031f8f.3 for ; Mon, 11 Dec 2023 03:17:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1702293470; x=1702898270; darn=sourceware.org; h=content-transfer-encoding:in-reply-to:autocrypt:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=6isCV69FRtVGBg8B9YTQWS7LJWuI8GO6OEuLXz6Ckbc=; b=IAzJQBATG5JEHiqFZwiLCV57mTa78XtDaLXmuDkmyDn/FOKFCRjiypOz0pgtoO9um+ jb3GKr3yv+yeW5fBpiNS6mQSM/AlbKrh1/uSvn2hu5Ns/aI4avumb6TNigPf+Yd2x04Z WSbS8SvMAsmvW1xBfUoU5tpP64DPcqoS8SXqjs2WDsV1AtyFqJgO9xDYs1pCjnZJDjgg /aBdmaLUIS9FfukKNJs7e+Zbtxn97ioLZO8ZLuX7ciB3E08n1TxPOaN6vCjpL1RePZTL GvvhbNJWkd7K7opY3mpqSbNxPALKsQCzmQg7wyw/ckUE3ZEh5xMFdu2pxH7kN02f2BDc US4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702293470; x=1702898270; h=content-transfer-encoding:in-reply-to:autocrypt:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=6isCV69FRtVGBg8B9YTQWS7LJWuI8GO6OEuLXz6Ckbc=; b=LC41xv30rWdcoOjF7WsDxFVAi/4iXhU1QwCDewgbTnSQuBmEGDaJA0gkYM5QA730xs 9l/vlqVSGD4n8j7XKpZPYfbu9ii9C49BENNAfWyn2r7nwF03g9tp42JwzR9QaPGXsjg1 NYVlfWHDRcQpLGSxpKgq+uDtpZ+EWkMuEp6QWBIdtYBpSmGHpwmCmGZY3UJfkafTx99G pVehaL8bV4leF1pKORBzk3jSK3CGso9Rr4ey1bcy2K0eQZ6d70IdSWwq4YGSYQJ+J6K6 xL5cc2yXo/3WasR8hOlb2FzPyWvHbyuTNsq7bVYU1rz8i8unyj/2E12hpR+/DwH9ySx9 XFmg== X-Gm-Message-State: AOJu0Yw0dS6BP6bE/cqg6L7xPlUzUueJG8dYD0QHc7RVCCmX9p3DXXTa 3GsASADJI3hOuDEq1N1eXlZC X-Google-Smtp-Source: AGHT+IGDgUSKiC3P3lVdKFVRpMWzy1gP7GqMMFocePn7y99t1+gFVhSCgXyT2xYNgRbzzq5QaMC/OQ== X-Received: by 2002:a5d:440c:0:b0:333:4243:8c47 with SMTP id z12-20020a5d440c000000b0033342438c47mr848424wrq.188.1702293470454; Mon, 11 Dec 2023 03:17:50 -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 k15-20020adfb34f000000b003334a1e92dasm8436345wrd.70.2023.12.11.03.17.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 11 Dec 2023 03:17:50 -0800 (PST) Message-ID: <5373d135-73d0-41db-a1cf-4079433ab379@suse.com> Date: Mon, 11 Dec 2023 12:17:30 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 7/9] Support APX Push2/Pop2 To: "Cui, Lili" , "Mo, Zewei" Cc: hongjiu.lu@intel.com, binutils@sourceware.org References: <20231124070213.3886483-1-lili.cui@intel.com> <20231124070213.3886483-7-lili.cui@intel.com> Content-Language: en-US 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: <20231124070213.3886483-7-lili.cui@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3026.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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 24.11.2023 08:02, Cui, Lili wrote: > --- a/gas/config/tc-i386.c > +++ b/gas/config/tc-i386.c > @@ -248,6 +248,7 @@ enum i386_error > invalid_vector_register_set, > invalid_tmm_register_set, > invalid_dest_and_src_register_set, > + invalid_src_register_set, > invalid_pseudo_prefix, > unsupported_vector_index_register, > unsupported_broadcast, > @@ -256,6 +257,7 @@ enum i386_error > mask_not_on_destination, > no_default_mask, > unsupported_rc_sae, > + unsupported_rsp_register, > invalid_register_operand, > internal_error, > }; > @@ -5398,6 +5400,9 @@ md_assemble (char *line) > case invalid_dest_and_src_register_set: > err_msg = _("destination and source registers must be distinct"); > break; > + case invalid_src_register_set: Did you mean invalid_dest_register_set and ... > + err_msg = _("two source registers must be distinct"); ... "two destination ..."? This is for POP2, after all, which has no source register at all. > @@ -5422,6 +5427,9 @@ md_assemble (char *line) > case unsupported_rc_sae: > err_msg = _("unsupported static rounding/sae"); > break; > + case unsupported_rsp_register: > + err_msg = _("cannot be used with %rsp register"); > + break; While this wording looks okay as visible here, please consider it in the context it is used in: "cannot be used with %rsp register for `push2'" is, I'm sorry to say that, clumsy at best. If you want to stick to setting err_msg, how about "%rsp register cannot be used"? Personally I'd prefer a resulting output of "%rsp register cannot be used with `push2'", but I wouldn't insist on you going that route if you don't like that. > @@ -7113,6 +7121,33 @@ check_EgprOperands (const insn_template *t) > return 0; > } > > +/* Check if APX operands are valid for the instruction. */ > +static int Please can functions returning boolean indicators have a return type of "bool" (and perhaps use "true" as the success indicator, not "false")? > +check_APX_operands (const insn_template *t) > +{ > + /* Push2* and Pop2* cannot use RSP and Pop2* cannot pop two same registers. > + */ > + if (t->mnem_off == MN_push2 || t->mnem_off == MN_push2p > + || t->mnem_off == MN_pop2 || t->mnem_off == MN_pop2p) Considering (perhaps just theoretical) further additions here, did you consider using switch()? Even without further additions this would imo be more legible (due to there being slightly less redundancy). > --- a/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s > +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s > @@ -28,3 +28,9 @@ _start: > .byte 0xff > #{evex} inc %rax %rbx EVEX.vvvv' != 1111 && EVEX.ND = 0. > .insn EVEX.L0.NP.M4.W1 0xff, %rax, %rbx > + .byte 0xff > + # pop2 %rax, %rbx set EVEX.ND=0. > + .byte 0x62,0xf4,0x64,0x08,0x8f,0xc0 > + .byte 0xff, 0xff, 0xff > + # pop2 %rax set EVEX.vvvv' = 1111. Another instance of the unclear EVEX.vvvv' (i.e. the questionable nature if ' here). Yet then - what is the test below checking? EVEX.vvvv encodes one of the two operands, so all values are valid? Isn't this about both operands being the same? That would better be said then explicitly, e.g. simply # pop2 %rax, %rax (twice same destination) > + .byte 0x62,0xf4,0x7c,0x18,0x8f,0xc0 Also again both new tests use .byte instead of .insn: Is there a particular reason? Here are a couple of examples that I have readily available (Intel syntax again, ftaod): .insn EVEX.L0.M4.W0 0x8f/0, r8, rax{sae} ; pop2 r8, rax .insn EVEX.L0.M4.W0 0x8f/0, xmm16, rax{sae} ; pop2 r16, rax .insn EVEX.L0.M4.W0 0x8f/0, rax, r8{sae} ; pop2 rax, r8 .insn EVEX.L0.M12.W0 0x8f/0, rax, rax{sae} ; pop2 rax, r16 .insn EVEX.L0.M4.W1 0x8f/0, rax, rcx{sae} ; pop2.x rax, rcx I'm sure you can derive from them what you're actually after. > --- /dev/null > +++ b/gas/testsuite/gas/i386/x86-64-apx-push2pop2.s > @@ -0,0 +1,39 @@ > +# Check 64bit APX-Push2Pop2 instructions > + > + .allow_index_reg > + .text > +_start: > + push2 %rbx, %rax > + push2 %r17, %r8 > + push2 %r9, %r31 > + push2 %r31, %r24 > + push2p %rbx, %rax > + push2p %r17, %r8 > + push2p %r9, %r31 > + push2p %r31, %r24 > + pop2 %rax, %rbx > + pop2 %r8, %r17 > + pop2 %r31, %r9 > + pop2 %r24, %r31 > + pop2p %rax, %rbx > + pop2p %r8, %r17 > + pop2p %r31, %r9 > + pop2p %r24, %r31 > + > +.intel_syntax noprefix Nit: Un-indented directive again. > --- a/opcodes/i386-dis.c > +++ b/opcodes/i386-dis.c > @@ -105,6 +105,7 @@ static bool FXSAVE_Fixup (instr_info *, int, int); > static bool MOVSXD_Fixup (instr_info *, int, int); > static bool DistinctDest_Fixup (instr_info *, int, int); > static bool PREFETCHI_Fixup (instr_info *, int, int); > +static bool PUSH2_POP2_Fixup (instr_info *, int, int); > > static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const disassemble_info *, > enum disassembler_style, > @@ -225,6 +226,9 @@ struct instr_info > } > vex; > > +/* For APX EVEX-promoted prefix, EVEX.ND shares the same bit as vex.b. */ > +#define nd b Can this be moved ahead to patch 4, such that it can be used there (instead of vex.b) as well? IOW ... > @@ -9125,7 +9133,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins) > > /* EVEX from legacy instructions, when the EVEX.ND bit is 0, > all bits of EVEX.vvvv and EVEX.V' must be 1. */ > - if (ins->evex_type == evex_from_legacy && !ins->vex.b > + if (ins->evex_type == evex_from_legacy && !ins->vex.nd > && (ins->vex.register_specifier || !ins->vex.v)) > return &bad_opcode; ... neither this nor ... > @@ -13388,11 +13396,10 @@ OP_VEX (instr_info *ins, int bytemode, int sizeflag ATTRIBUTE_UNUSED) > if (!ins->need_vex) > return true; > > - /* Here vex.b is treated as "EVEX.ND". */ > if (ins->evex_type == evex_from_legacy) > { > ins->evex_used |= EVEX_b_used; > - if (!ins->vex.b) > + if (!ins->vex.nd) > return true; > } ... this should require touching here. > @@ -13884,3 +13894,26 @@ PREFETCHI_Fixup (instr_info *ins, int bytemode, int sizeflag) > > return OP_M (ins, bytemode, sizeflag); > } > + > +static bool > +PUSH2_POP2_Fixup (instr_info *ins, int bytemode, int sizeflag) > +{ > + if (ins->modrm.mod != 3 || !ins->vex.b) Did you mean vex.nd? Plus, considering the vex.nd check further down, why is this checked both here and there? > + return true; Doesn't this result in silently bogus/wrong output? Shouldn't you print "(bad)" like you do further down? At which point it may make sense to simply fold both if()s? > --- a/opcodes/i386-opc.h > +++ b/opcodes/i386-opc.h > @@ -807,6 +807,7 @@ typedef struct i386_opcode_modifier > unsigned int isa64:2; > unsigned int noegpr:1; > unsigned int nf:1; > + unsigned int push2pop2:1; > } i386_opcode_modifier; Still a new modifier despite my earlier request to avoid adding one when you easily can? Here OperandConstraint is actually fully applicable to use, as what you want to enforce is a constraint on operands. Jan