Re: [PATCH,V4 4/8] opcodes: aarch64: flags to denote subclasses of arithmetic insns

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

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