From: Richard Sandiford <richard.sandiford@arm.com>
To: Indu Bhagat <indu.bhagat@oracle.com>
Cc: binutils@sourceware.org, Richard.Earnshaw@arm.com
Subject: Re: [PATCH,V4 4/8] opcodes: aarch64: flags to denote subclasses of arithmetic insns
Date: Thu, 11 Jul 2024 19:43:11 +0100 [thread overview]
Message-ID: <mptwmlrpy68.fsf@arm.com> (raw)
In-Reply-To: <04067366-d397-4366-8a82-21767180ae64@oracle.com> (Indu Bhagat's message of "Thu, 11 Jul 2024 10:58:19 -0700")
Indu Bhagat <indu.bhagat@oracle.com> writes:
> On 7/11/24 05:52, Richard Sandiford wrote:
>> Indu Bhagat <indu.bhagat@oracle.com> writes:
>>> On 7/1/24 11:13, Richard Sandiford wrote:
>>>> Indu Bhagat <indu.bhagat@oracle.com> 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
next prev parent reply other threads:[~2024-07-11 18:43 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-01 2:53 [PATCH,V4 0/8] Add SCFI support for aarch64 Indu Bhagat
2024-07-01 2:53 ` [PATCH,V4 1/8] gas: scfi: make scfi_state_restore_reg function more precise Indu Bhagat
2024-07-12 15:03 ` [PATCH, V4 " Indu Bhagat
2024-07-01 2:53 ` [PATCH,V4 2/8] include: opcodes: aarch64: define new subclasses Indu Bhagat
2024-07-01 17:40 ` Richard Sandiford
2024-07-11 5:14 ` Indu Bhagat
2024-07-11 12:22 ` Richard Sandiford
2024-07-11 17:59 ` Indu Bhagat
2024-07-01 2:53 ` [PATCH,V4 3/8] opcodes: aarch64: flags to denote subclasses of ldst insns Indu Bhagat
2024-07-01 18:06 ` Richard Sandiford
2024-07-11 5:45 ` Indu Bhagat
2024-07-12 13:59 ` Indu Bhagat
2024-07-13 7:34 ` Indu Bhagat
2024-07-01 2:54 ` [PATCH,V4 4/8] opcodes: aarch64: flags to denote subclasses of arithmetic insns Indu Bhagat
2024-07-01 18:13 ` Richard Sandiford
2024-07-11 5:47 ` Indu Bhagat
2024-07-11 12:52 ` Richard Sandiford
2024-07-11 17:58 ` Indu Bhagat
2024-07-11 18:43 ` Richard Sandiford [this message]
2024-07-12 12:53 ` Indu Bhagat
2024-07-01 2:54 ` [PATCH,V4 5/8] opcodes: aarch64: flags to denote subclasses of uncond branches Indu Bhagat
2024-07-01 2:54 ` [PATCH,V4 6/8] opcodes: aarch64: enforce checks on subclass flags in aarch64-gen.c Indu Bhagat
2024-07-01 2:54 ` [PATCH,V4 7/8] gas: aarch64: add experimental support for SCFI Indu Bhagat
2024-07-01 19:49 ` Richard Sandiford
2024-07-11 6:30 ` Indu Bhagat
2024-07-11 13:15 ` Richard Sandiford
2024-07-11 19:07 ` Indu Bhagat
2024-07-11 20:10 ` Richard Sandiford
2024-07-01 2:54 ` [PATCH,V4 8/8] gas: aarch64: testsuite: add new tests " Indu Bhagat
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=mptwmlrpy68.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=Richard.Earnshaw@arm.com \
--cc=binutils@sourceware.org \
--cc=indu.bhagat@oracle.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).