Re: [PATCH,V4 2/8] include: opcodes: aarch64: define new subclasses

[Indu Bhagat <indu.bhagat@oracle.com>]

On 7/11/24 05:22, Richard Sandiford wrote:
> 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.
> 

Thanks. I too spotted this one and made that change already after I sent 
the email yesterday :)