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

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

On 7/12/24 6:59 AM, Indu Bhagat wrote:
> On 7/10/24 22:45, Indu Bhagat wrote:
>> On 7/1/24 11:06, Richard Sandiford wrote:
>>> Indu Bhagat <indu.bhagat@oracle.com> writes:
>>>> [Changes in V4]
>>>>   - Specify subclasses only for those iclasses relevent to SCFI:
>>>>        ldst_imm9, ldst_pos, ldstpair_indexed, ldstpair_off
>>>> [End of changes in V4]
>>>>
>>>> [Changes in V3]
>>>> - Use F_LDST_SWAP for lse_atomic ld/st ops.  Use of F_LDST_LOAD or
>>>>    F_LDST_STORE was incorrect.
>>>> [End of changes in V3]
>>>>
>>>> [New in V2]
>>>>
>>>> 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.
>>>>
>>>> opcodes/
>>>>      * aarch64-tbl.h: Use the new F_LDST_* flags.
>>>> ---
>>>>   opcodes/aarch64-tbl.h | 78 
>>>> +++++++++++++++++++++----------------------
>>>>   1 file changed, 39 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h
>>>> index 7ef9cea9119..6e523db6277 100644
>>>> --- a/opcodes/aarch64-tbl.h
>>>> +++ b/opcodes/aarch64-tbl.h
>>>> @@ -4123,39 +4123,39 @@ const struct aarch64_opcode 
>>>> aarch64_opcode_table[] =
>>>>     __FP_INSN ("fcsel", 0x1e200c00, 0xff200c00, floatsel, 0, OP4 
>>>> (Fd, Fn, Fm, COND), QL_FP_COND, F_FPTYPE),
>>>>     FF16_INSN ("fcsel", 0x1ee00c00, 0xff200c00, floatsel, OP4 (Fd, 
>>>> Fn, Fm, COND), QL_FP_COND_H, F_FPTYPE),
>>>>     /* Load/store register (immediate indexed).  */
>>>> -  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W8, 0),
>>>> -  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W8, 0),
>>>> -  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_R8, F_LDS_SIZE),
>>>> -  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>>> ADDR_SIMM9), QL_LDST_FP, 0),
>>>> -  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>>> ADDR_SIMM9), QL_LDST_FP, 0),
>>>> -  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W16, 0),
>>>> -  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W16, 0),
>>>> -  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_R16, F_LDS_SIZE),
>>>> -  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
>>>> -  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_R, F_GPRSIZE_IN_Q),
>>>> -  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_X32, 0),
>>>> +  CORE_INSN ("strb", 0x38000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W8, F_LDST_STORE),
>>>> +  CORE_INSN ("ldrb", 0x38400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W8, F_LDST_LOAD),
>>>> +  CORE_INSN ("ldrsb", 0x38800400, 0xffa00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_R8, F_LDST_LOAD | F_LDS_SIZE),
>>>> +  CORE_INSN ("str", 0x3c000400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>>> ADDR_SIMM9), QL_LDST_FP, F_LDST_STORE),
>>>> +  CORE_INSN ("ldr", 0x3c400400, 0x3f600400, ldst_imm9, 0, OP2 (Ft, 
>>>> ADDR_SIMM9), QL_LDST_FP, F_LDST_LOAD),
>>>> +  CORE_INSN ("strh", 0x78000400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W16, F_LDST_STORE),
>>>> +  CORE_INSN ("ldrh", 0x78400400, 0xffe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_W16, F_LDST_LOAD),
>>>> +  CORE_INSN ("ldrsh", 0x78800400, 0xffa00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_R16, F_LDST_LOAD | F_LDS_SIZE),
>>>> +  CORE_INSN ("str", 0xb8000400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_R, F_LDST_STORE | F_GPRSIZE_IN_Q),
>>>> +  CORE_INSN ("ldr", 0xb8400400, 0xbfe00400, ldst_imm9, 0, OP2 (Rt, 
>>>> ADDR_SIMM9), QL_LDST_R, F_LDST_LOAD | F_GPRSIZE_IN_Q),
>>>> +  CORE_INSN ("ldrsw", 0xb8800400, 0xffe00400, ldst_imm9, 0, OP2 
>>>> (Rt, ADDR_SIMM9), QL_LDST_X32, F_LDST_LOAD),
>>>>     /* Load/store Allocation Tag instructions.  */
>>>>     MEMTAG_INSN ("stg",  0xd9200800, 0xffe00c00, ldst_unscaled, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>>     MEMTAG_INSN ("stzg", 0xd9600800, 0xffe00c00, ldst_unscaled, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>>     MEMTAG_INSN ("st2g", 0xd9a00800, 0xffe00c00, ldst_unscaled, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>>     MEMTAG_INSN ("stz2g",0xd9e00800, 0xffe00c00, ldst_unscaled, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>> -  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>> -  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>> -  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>> -  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, 0),
>>>> +  MEMTAG_INSN ("stg",  0xd9200400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>>> +  MEMTAG_INSN ("stzg", 0xd9600400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>>> +  MEMTAG_INSN ("st2g", 0xd9a00400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>>> +  MEMTAG_INSN ("stz2g",0xd9e00400, 0xffe00400, ldst_imm9, OP2 
>>>> (Rt_SP, ADDR_SIMM13), QL_LDST_AT, F_LDST_STORE),
>>>
>>> These store an allocation tag, rather than storing the contents of the
>>> registers to regular memory.  I think they should be put in an "other"
>>> category.
>>>
>>
>> I have changed these to F_SUBCLASS_OTHER. And on the ginsn generation 
>> side, no memory ginsn op is generated (address calculation op is 
>> generated if writeback is in effect as usual).  Also added code 
>> comments   in tc-aarch-ginsn.c to clarify that skipping memory ops is 
>> needed because the memory op affects the tag only.
>>
>>>>     /* Load/store register (unsigned immediate).  */
>>>> -  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
>>>> -  CORE_INSN ("ldrb", 0x39400000, 0xffc00000, ldst_pos, OP_LDRB_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, 0),
>>>> -  CORE_INSN ("ldrsb", 0x39800000, 0xff800000, ldst_pos, 
>>>> OP_LDRSB_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R8, F_LDS_SIZE),
>>>> -  CORE_INSN ("str", 0x3d000000, 0x3f400000, ldst_pos, OP_STRF_POS, 
>>>> OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
>>>> -  CORE_INSN ("ldr", 0x3d400000, 0x3f400000, ldst_pos, OP_LDRF_POS, 
>>>> OP2 (Ft, ADDR_UIMM12), QL_LDST_FP, 0),
>>>> -  CORE_INSN ("strh", 0x79000000, 0xffc00000, ldst_pos, OP_STRH_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
>>>> -  CORE_INSN ("ldrh", 0x79400000, 0xffc00000, ldst_pos, OP_LDRH_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W16, 0),
>>>> -  CORE_INSN ("ldrsh", 0x79800000, 0xff800000, ldst_pos, 
>>>> OP_LDRSH_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_R16, F_LDS_SIZE),
>>>> -  CORE_INSN ("str", 0xb9000000, 0xbfc00000, ldst_pos, OP_STR_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
>>>> -  CORE_INSN ("ldr", 0xb9400000, 0xbfc00000, ldst_pos, OP_LDR_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_R, F_GPRSIZE_IN_Q),
>>>> -  CORE_INSN ("ldrsw", 0xb9800000, 0xffc00000, ldst_pos, 
>>>> OP_LDRSW_POS, OP2 (Rt, ADDR_UIMM12), QL_LDST_X32, 0),
>>>> -  CORE_INSN ("prfm", 0xf9800000, 0xffc00000, ldst_pos, OP_PRFM_POS, 
>>>> OP2 (PRFOP, ADDR_UIMM12), QL_LDST_PRFM, 0),
>>>> +  CORE_INSN ("strb", 0x39000000, 0xffc00000, ldst_pos, OP_STRB_POS, 
>>>> OP2 (Rt, ADDR_UIMM12), QL_LDST_W8, F_LDST_STORE),
>>>
>>> AFAICT, patch 7/8 doesn't distinguish between byte stores and other 
>>> sizes
>>> of store, even though this matters for CFI purposes.  I think we should
>>> use an "other" category for anything that isn't a full load or store of
>>> a W register, X register, or FPR.
>>>
>>> Patch 7/8 shouldn't treat a W, B, H or S load or store as being enough
>>> for CFI purposes.  The width has to be 8 bytes or more.
>>>
>>> Similarly, for LDP & STP, 7/8 should punt on W and S loads and stores,
>>> since they cannot implement a full save & restore for CFI purposes.
>>>
>>
>> Currently, the checks in dw2gencfi.c issue a warning if CFI offsets 
>> are not a multiple of 8.  So if user says --scfi=experimental for
>>      stp     s8, s9, [sp, 96]
>> they get an error:
>>      Error: register save offset not a multiple of 8
>>
>> I was sort of relying on the above for some cases, while leaving the 
>> users on their own for other cases. E.g, No error is issued if the 
>> user does a save/restore of S register like so:
>>      str    s8, [sp, 96]
>>
>> Hmm, I could punt on W and S by not creating ginsn mem ops for them 
>> altogether.  Should the user be warned though ? How about something 
>> like :
>>     Warning: ignored probable save/restore op of less than 8-byte
>>
>> As this will be diagnosed by the ginsn creation code, the warning will 
>> be issued when a load / store:
>>    - of callee-saved register,
>>    - of width < 8 bytes,
>>    - and with addr_reg involving REG_SP or REG_FP is seen
>>
>> To avoid the high number of warnings, we could warn once (for the 
>> first sub 8-byte applicable load/store). Do you think the warning is 
>> useful ?
>>
> 
> I am inclining towards working on this as a follow-up patch (not 
> included in V5).  Punting on S and W registers should be easy, but I 
> would like to test it out to be sure I am not misssing anything.
> 
> Further, I am tending towards not adding a new warning for this in the 
> current infrastructure at all (mostly because there will be false 
> positives which dilutes the eficacy of the warning).  IIUC, the usage of 
> W and S registers to save/restore callee-saved registers is not 
> ABI-conformant anyway.
> 
> Please let me know your opinion on defering this as a follow-up patch.
> 

This is hopefully addressed in V5 that I will be sending soon.

No warning yet, but we will now punt on S and W registers.

Thanks
Indu