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 2/8] include: opcodes: aarch64: define new subclasses
Date: Thu, 11 Jul 2024 13:22:15 +0100 [thread overview]
Message-ID: <mptwmlsw22w.fsf@arm.com> (raw)
In-Reply-To: <8f282d4f-8402-429c-967d-994ff23114ec@oracle.com> (Indu Bhagat's message of "Wed, 10 Jul 2024 22:14:26 -0700")
Indu Bhagat <indu.bhagat@oracle.com> writes:
> On 7/1/24 10:40, Richard Sandiford wrote:
>> Indu Bhagat <indu.bhagat@oracle.com> writes:
>>> [New in V4]
>>>
>>> The existing iclass information tells us the general shape and purpose
>>> of the instructions. In some cases, however, we need to further disect
>>> the iclass on the basis of other finer-grain information. E.g., for the
>>> purpose of SCFI, we need to know whether a given insn with iclass of
>>> ldst_* is a load or a store. Similarly, whether a particular arithmetic
>>> insn is an add or sub or mov, etc.
>>>
>>> This patch defines new flags to demarcate the insns. Also provide an
>>> access function for subclass lookup.
>>>
>>> Later, we will enforce (in aarch64-gen.c) that if an iclass has at least
>>> one instruction with a non-zero subclass, all instructions of the iclass
>>> must have a non-zero subclass information. If none of the defined
>>> subclasses are applicable (or not required for SCFI purposes),
>>> F_SUBCLASS_OTHER can be used for such instructions.
>>>
>>> include/
>>> * opcode/aarch64.h (F_SUBCLASS): New flag.
>>> (F_SUBCLASS_OTHER): Likewise.
>>> (F_LDST_LOAD): Likewise.
>>> (F_LDST_STORE): Likewise.
>>> (F_LDST_SWAP): Likewise.
>>> (F_ARITH_ADD): Likewise.
>>> (F_ARITH_SUB): Likewise.
>>> (F_ARITH_MOV): Likewise.
>>> (F_BRANCH_CALL): Likewise.
>>> (F_BRANCH_RET): Likewise.
>>> (F_MAX_SUBCLASS): Likewise.
>>> (aarch64_opcode_subclass_p): New definition.
>>> ---
>>> include/opcode/aarch64.h | 32 +++++++++++++++++++++++++++++++-
>>> 1 file changed, 31 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/opcode/aarch64.h b/include/opcode/aarch64.h
>>> index 61758c96285..c3ac1326b9c 100644
>>> --- a/include/opcode/aarch64.h
>>> +++ b/include/opcode/aarch64.h
>>> @@ -1359,7 +1359,31 @@ extern const aarch64_opcode aarch64_opcode_table[];
>>> #define F_OPD_SIZE (1ULL << 34)
>>> /* RCPC3 instruction has the field of 'size'. */
>>> #define F_RCPC3_SIZE (1ULL << 35)
>>> -/* Next bit is 36. */
>>> +
>>> +/* 4-bit flag field to indicate subclass of instructions.
>>> + The available 14 nonzero values are consecutive, with room for
>>> + more subclasses in future. */
>>> +#define F_SUBCLASS (15ULL << 36)
>>> +
>>> +#define F_SUBCLASS_OTHER (F_SUBCLASS)
>>> +
>>> +#define F_LDST_LOAD (1ULL << 36)
>>> +#define F_LDST_STORE (2ULL << 36)
>>> +/* A load followed by a store (using the same address). */
>>> +#define F_LDST_SWAP (3ULL << 36)
>>> +/* Subclasses to denote add, sub and mov insns. */
>>> +#define F_ARITH_ADD (4ULL << 36)
>>> +#define F_ARITH_SUB (5ULL << 36)
>>> +#define F_ARITH_MOV (6ULL << 36)
>>> +/* Subclasses to denote call and ret insns. */
>>> +#define F_BRANCH_CALL (7ULL << 36)
>>> +#define F_BRANCH_RET (8ULL << 36)
>>
>> When I said this field was an enum, I should have said that it's
>> a class-specific enum. I don't think we need the ldst and arith
>> subclasses to use independent values, and it might be better to
>> allow overlap from the outset, rather than try to retrofit it later.
>>
>> F_LDST_SWAP is no longer used (a good thing!)
>>
>
> OK. I have switched it to iclass-specific enum with overlap. I have
> removed F_LDST_SWAP and F_MAX_SUBLASS as they are unused.
>
> I have added function-level comments:
>
> /* Whether the opcode has the specific subclass flag.
> N.B. The overlap between F_LDST_*, F_ARITH_*, and F_BRANCH_* flags means
> that the callers of this function have the responsibility of
> checking for
> the flags appropriate for the specific iclass. */
> static inline bool
> aarch64_opcode_subclass_p (const aarch64_opcode* opcode, uint64_t flag)
> {
> return ((opcode->flags & F_SUBCLASS) == flag);
> }
Sounds good. How about "The overlap between flags like ... means that ...",
to make it clear that the list isn't meant to be exhaustive? We could add
other such subclasses in future.
Richard
>> Thanks,
>> Richard
>>
>>> +
>>> +#define F_SUBCLASS_OTHER (F_SUBCLASS)
>>> +/* PS: To perform some sanity checks, the last declared subclass flag is used.
>>> + Keep F_MAX_SUBCLASS updated when declaring new subclasses. */
>>> +#define F_MAX_SUBCLASS (F_BRANCH_RET)
>>> +/* Next bit is 40. */
>>>
>>> /* Instruction constraints. */
>>> /* This instruction has a predication constraint on the instruction at PC+4. */
>>> @@ -1398,6 +1422,12 @@ pseudo_opcode_p (const aarch64_opcode *opcode)
>>> return (opcode->flags & F_PSEUDO) != 0lu;
>>> }
>>>
>>> +static inline bool
>>> +aarch64_opcode_subclass_p (const aarch64_opcode* opcode, uint64_t flag)
>>> +{
>>> + return ((opcode->flags & F_SUBCLASS) == flag);
>>> +}
>>> +
>>> /* Deal with two possible scenarios: If F_OP_PAIR_OPT not set, as is the case
>>> by default, F_OPDn_OPT must equal IDX + 1, else F_OPDn_OPT must be in range
>>> [IDX, IDX + 1]. */
next prev parent reply other threads:[~2024-07-11 12:22 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 [this message]
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
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=mptwmlsw22w.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).