[PATCH,V4 3/8] opcodes: aarch64: flags to denote subclasses of ldst insns
Indu Bhagat
indu.bhagat@oracle.com
Sat Jul 13 07:34:37 GMT 2024
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
More information about the Binutils
mailing list