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

[Richard Sandiford <richard.sandiford@arm.com>]

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].  */