From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 59602387545A for ; Thu, 11 Jul 2024 18:43:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 59602387545A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 59602387545A Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1720723401; cv=none; b=O0DuZNtgDoG5EQIp7NInbqt3tCN4vUVe4o+zrNlfi2+EH1L3VnwBpadsdX6MAirApo3ON/WnmP4hO0zx36ARR5e1xRw+G9rsNCy1x4EDH6oNItgIy/t0V3T6WUzZ4zJ2BA1E/UU4URCbS9n+XAZIaJblzbLM878bnoLF+DUe+Yk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1720723401; c=relaxed/simple; bh=Xd1CFxErRN0NFJzAk28zu31CbE8gfotJ9emYmcS4i/c=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=ISh+1xG+nb5jh7x0LKaRfHfbN4NOlwCE0FVBNioJZgxE1jd1RGdow3xaTMybi/WkyoMT2hgDdElsEzGVPhtW1BcHwQq5EtCmnPV7/j0CZV2fJvauShpnEnfZ0YbYNPOYckd03oCMr8I3biSgcT5iLLIhViX7oFvvRvjklKZxK3o= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4FDD51007; Thu, 11 Jul 2024 11:43:39 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 603313F766; Thu, 11 Jul 2024 11:43:13 -0700 (PDT) From: Richard Sandiford To: Indu Bhagat Mail-Followup-To: Indu Bhagat ,binutils@sourceware.org, Richard.Earnshaw@arm.com, richard.sandiford@arm.com Cc: binutils@sourceware.org, Richard.Earnshaw@arm.com Subject: Re: [PATCH,V4 4/8] opcodes: aarch64: flags to denote subclasses of arithmetic insns In-Reply-To: <04067366-d397-4366-8a82-21767180ae64@oracle.com> (Indu Bhagat's message of "Thu, 11 Jul 2024 10:58:19 -0700") References: <20240701025404.3361349-1-indu.bhagat@oracle.com> <20240701025404.3361349-5-indu.bhagat@oracle.com> <04067366-d397-4366-8a82-21767180ae64@oracle.com> Date: Thu, 11 Jul 2024 19:43:11 +0100 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-19.4 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,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: Indu Bhagat writes: > On 7/11/24 05:52, Richard Sandiford wrote: >> Indu Bhagat writes: >>> On 7/1/24 11:13, Richard Sandiford wrote: >>>> Indu Bhagat writes: >>>>> [Changes in V4] >>>>> - Specify subclasses only for those iclasses relevent to SCFI: >>>>> addsub_imm, and addsub_ext >>>>> [End of changes in V4] >>>>> >>>>> [No changes in V3] >>>>> [New in V2] >>>>> >>>>> Use the three new subclass flags: F_ARITH_ADD, F_ARITH_SUB, >>>>> F_ARITH_MOV, to indicate add, sub and mov ops respectively. >>>>> >>>>> opcodes/ >>>>> * aarch64-tbl.h: Use the new F_ARITH_* flags. >>>>> --- >>>>> opcodes/aarch64-tbl.h | 30 +++++++++++++++--------------- >>>>> 1 file changed, 15 insertions(+), 15 deletions(-) >>>>> >>>>> diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h >>>>> index 6e523db6277..57727254d43 100644 >>>>> --- a/opcodes/aarch64-tbl.h >>>>> +++ b/opcodes/aarch64-tbl.h >>>>> @@ -3205,22 +3205,22 @@ const struct aarch64_opcode aarch64_opcode_table[] = >>>>> CORE_INSN ("sbcs", 0x7a000000, 0x7fe0fc00, addsub_carry, 0, OP3 (Rd, Rn, Rm), QL_I3SAMER, F_HAS_ALIAS | F_SF), >>>>> CORE_INSN ("ngcs", 0x7a0003e0, 0x7fe0ffe0, addsub_carry, 0, OP2 (Rd, Rm), QL_I2SAME, F_ALIAS | F_SF), >>>>> /* Add/subtract (extended register). */ >>>>> - CORE_INSN ("add", 0x0b200000, 0x7fe00000, addsub_ext, 0, OP3 (Rd_SP, Rn_SP, Rm_EXT), QL_I3_EXT, F_SF), >>>>> - CORE_INSN ("adds", 0x2b200000, 0x7fe00000, addsub_ext, 0, OP3 (Rd, Rn_SP, Rm_EXT), QL_I3_EXT, F_HAS_ALIAS | F_SF), >>>>> - CORE_INSN ("cmn", 0x2b20001f, 0x7fe0001f, addsub_ext, 0, OP2 (Rn_SP, Rm_EXT), QL_I2_EXT, F_ALIAS | F_SF), >>>>> - CORE_INSN ("sub", 0x4b200000, 0x7fe00000, addsub_ext, 0, OP3 (Rd_SP, Rn_SP, Rm_EXT), QL_I3_EXT, F_SF), >>>>> - CORE_INSN ("subs", 0x6b200000, 0x7fe00000, addsub_ext, 0, OP3 (Rd, Rn_SP, Rm_EXT), QL_I3_EXT, F_HAS_ALIAS | F_SF), >>>>> - CORE_INSN ("cmp", 0x6b20001f, 0x7fe0001f, addsub_ext, 0, OP2 (Rn_SP, Rm_EXT), QL_I2_EXT, F_ALIAS | F_SF), >>>>> + CORE_INSN ("add", 0x0b200000, 0x7fe00000, addsub_ext, 0, OP3 (Rd_SP, Rn_SP, Rm_EXT), QL_I3_EXT, F_ARITH_ADD | F_SF), >>>>> + CORE_INSN ("adds", 0x2b200000, 0x7fe00000, addsub_ext, 0, OP3 (Rd, Rn_SP, Rm_EXT), QL_I3_EXT, F_ARITH_ADD | F_HAS_ALIAS | F_SF), >>>>> + CORE_INSN ("cmn", 0x2b20001f, 0x7fe0001f, addsub_ext, 0, OP2 (Rn_SP, Rm_EXT), QL_I2_EXT, F_SUBCLASS_OTHER | F_ALIAS | F_SF), >>>>> + CORE_INSN ("sub", 0x4b200000, 0x7fe00000, addsub_ext, 0, OP3 (Rd_SP, Rn_SP, Rm_EXT), QL_I3_EXT, F_ARITH_SUB | F_SF), >>>>> + CORE_INSN ("subs", 0x6b200000, 0x7fe00000, addsub_ext, 0, OP3 (Rd, Rn_SP, Rm_EXT), QL_I3_EXT, F_ARITH_SUB | F_HAS_ALIAS | F_SF), >>>>> + CORE_INSN ("cmp", 0x6b20001f, 0x7fe0001f, addsub_ext, 0, OP2 (Rn_SP, Rm_EXT), QL_I2_EXT, F_SUBCLASS_OTHER | F_ALIAS | F_SF), >>>>> /* Add/subtract (immediate). */ >>>>> - CORE_INSN ("add", 0x11000000, 0x7f000000, addsub_imm, OP_ADD, OP3 (Rd_SP, Rn_SP, AIMM), QL_R2NIL, F_HAS_ALIAS | F_SF), >>>>> - CORE_INSN ("mov", 0x11000000, 0x7ffffc00, addsub_imm, 0, OP2 (Rd_SP, Rn_SP), QL_I2SP, F_ALIAS | F_SF), >>>>> - CORE_INSN ("adds", 0x31000000, 0x7f000000, addsub_imm, 0, OP3 (Rd, Rn_SP, AIMM), QL_R2NIL, F_HAS_ALIAS | F_SF), >>>>> - CORE_INSN ("cmn", 0x3100001f, 0x7f00001f, addsub_imm, 0, OP2 (Rn_SP, AIMM), QL_R1NIL, F_ALIAS | F_SF), >>>>> - CORE_INSN ("sub", 0x51000000, 0x7f000000, addsub_imm, 0, OP3 (Rd_SP, Rn_SP, AIMM), QL_R2NIL, F_SF), >>>>> - CORE_INSN ("subs", 0x71000000, 0x7f000000, addsub_imm, 0, OP3 (Rd, Rn_SP, AIMM), QL_R2NIL, F_HAS_ALIAS | F_SF), >>>>> - CORE_INSN ("cmp", 0x7100001f, 0x7f00001f, addsub_imm, 0, OP2 (Rn_SP, AIMM), QL_R1NIL, F_ALIAS | F_SF), >>>>> - MEMTAG_INSN ("addg", 0x91800000, 0xffc0c000, addsub_imm, OP4 (Rd_SP, Rn_SP, UIMM10, UIMM4_ADDG), QL_ADDG, 0), >>>>> - MEMTAG_INSN ("subg", 0xd1800000, 0xffc0c000, addsub_imm, OP4 (Rd_SP, Rn_SP, UIMM10, UIMM4_ADDG), QL_ADDG, 0), >>>>> + CORE_INSN ("add", 0x11000000, 0x7f000000, addsub_imm, OP_ADD, OP3 (Rd_SP, Rn_SP, AIMM), QL_R2NIL, F_ARITH_ADD | F_HAS_ALIAS | F_SF), >>>>> + CORE_INSN ("mov", 0x11000000, 0x7ffffc00, addsub_imm, 0, OP2 (Rd_SP, Rn_SP), QL_I2SP, F_ARITH_MOV | F_ALIAS | F_SF), >>>>> + CORE_INSN ("adds", 0x31000000, 0x7f000000, addsub_imm, 0, OP3 (Rd, Rn_SP, AIMM), QL_R2NIL, F_ARITH_ADD | F_HAS_ALIAS | F_SF), >>>>> + CORE_INSN ("cmn", 0x3100001f, 0x7f00001f, addsub_imm, 0, OP2 (Rn_SP, AIMM), QL_R1NIL, F_SUBCLASS_OTHER | F_ALIAS | F_SF), >>>>> + CORE_INSN ("sub", 0x51000000, 0x7f000000, addsub_imm, 0, OP3 (Rd_SP, Rn_SP, AIMM), QL_R2NIL, F_ARITH_SUB | F_SF), >>>>> + CORE_INSN ("subs", 0x71000000, 0x7f000000, addsub_imm, 0, OP3 (Rd, Rn_SP, AIMM), QL_R2NIL, F_ARITH_SUB | F_HAS_ALIAS | F_SF), >>>>> + CORE_INSN ("cmp", 0x7100001f, 0x7f00001f, addsub_imm, 0, OP2 (Rn_SP, AIMM), QL_R1NIL, F_SUBCLASS_OTHER | F_ALIAS | F_SF), >>>>> + MEMTAG_INSN ("addg", 0x91800000, 0xffc0c000, addsub_imm, OP4 (Rd_SP, Rn_SP, UIMM10, UIMM4_ADDG), QL_ADDG, F_ARITH_ADD), >>>>> + MEMTAG_INSN ("subg", 0xd1800000, 0xffc0c000, addsub_imm, OP4 (Rd_SP, Rn_SP, UIMM10, UIMM4_ADDG), QL_ADDG, F_ARITH_SUB), >>>> >>>> I suppose this raises the question: is GINSN_TYPE_ADD specifically >>>> for address arithmetic, or is it a normal addition? If it's a >>>> normal addition then ADDG doesn't really fit. If it's address >>>> arithmetic then it might be worth making the names more explicit. >>>> >>> > > > Apologies, my brain tricked me into reading the above as "is F_ARITH_ADD > specifically for address arithmetic or is it normal addition?" all this > time until now, and hence, what might appear as a confused reply in the > previous email. > > Hopefully I have not digressed too far. > >>> For SCFI, we are interested in the insns which may have manipulated >>> REG_SP and REG_FP, so the intention is around address arithmetic. >>> >>> ATM, we generate ginsn for _all_ add, sub (and mov) in the iclass >>> addsub_imm, addsub_ext (and movewide..), irrespective of whether the >>> destination is REG_SP/REG_FP or not. >>> >>> IOW, "keep the ginsn creation code not tied to GINSN_GEN_SCFI" has been >>> followed where affordable. >> >> I think this is useful even for SCFI, since large stack allocations could >> be done using intermediate calculations into temporary registers. >> > > Yes to using intermediate calculations into temporary registers. This > is true especially for aarch64 where we may see patterns like: > > mov x16, 4384 > add sp, sp, x16 > > (which I would like to support for SCFI/aarch64 next. Adding support > for this means enabling some data flow for sp traceability; SCFI does > not have much of data flow ATM.) > > But I am not sure if the GINSN_TYPE_ADD / GINSN_TYPE_ADD_ADDR > demarcation will be useful for SCFI because: effectively GINSN_TYPE_ADD > with dest as REG_SP/REG_FP is address arithmetic for SCFI. SCFI can (and > does) ignore the rest of GINSN_TYPE_ADD / GINSN_TYPE_SUB ops. > >>> I dont have a good new name (F_ADDR_ARITH_* ?); I personally find >>> F_ADDR_ARITH_* unsuitable because this new name ties the >>> subclassification to the current usecase (SCFI and its need to see those >>> address arithmetic). But may be I am overthinking. >>> >>> If you have a suggestion, please let me know. >> >> The distinction between a full addition and address addition sounds >> like it could be generally useful, beyond just SCFI. How abot having >> both GINSN_TYPE_ADD and GINSN_TYPE_ADD_ADDR? Places that are only >> interested in address arithmetic can treat both types in the same way. >> Types that want natural addition can handle GINSN_TYPE_ADD only. >> > > The distinction may be useful for usecases other than SCFI, true; > depending on the usecase. > > I think my nervousness with the explicit demarcation using two type of > ginsns, GINSN_TYPE_ADD and GINSN_TYPE_ADD_ADDR now, is on the generation > side: > > - Wont we simply look at the destination register being REG_SP / > REG_FP in some cases to pick GINSN_TYPE_ADD_ADDR instead of > GINSN_TYPE_ADD. If so, GINSN_TYPE_ADD_ADDR contains no more information > than GINSN_TYPE_ADD in those cases. I was thinking the distinction between GINSN_TYPE_ADD and GINSN_TYPE_ADD_ADDR would be based on the opcode rather than the register. ADDG would be GINSN_TYPE_ADD_ADDR (since it adds "enough bits" to support address arithmetic, but does not carry into the tag bits). ADD would instead be GINSN_TYPE_ADD (since it's a full-register add). Both mappings would be independent of the destination register. I'm just nervous about saying that ADD and ADDG are the same operation, unless that operation is specifically described as being for addresses only. Another option would be to leave ADDG and SUBG as "other" for the initial patch and deal with them as a follow-on. > - Also, for other ISAs, I am not sure ATM if this will make things > more complicated on the ginsn creation side as other rules to pick > GINSN_TYPE_ADD_ADDR vs GINSN_TYPE_ADD may be involved. And not all might > be implementable at the time of ginsn creation (as some def-use analysis > may be necessary to demarcate them?) I think all other ISAs would just use GINSN_TYPE_ADD, unless they're doing a similar tag operation to ADDG/SUBG. Thanks, Richard