From: Richard Sandiford <richard.sandiford@arm.com>
To: Victor Do Nascimento <victor.donascimento@arm.com>
Cc: <gcc-patches@gcc.gnu.org>, <kyrylo.tkachov@arm.com>,
<Richard.Earnshaw@arm.com>
Subject: Re: [PATCH 4/5] aarch64: Implement 128-bit extension to ACLE sysreg r/w builtins
Date: Tue, 07 Nov 2023 23:14:31 +0000 [thread overview]
Message-ID: <mptcywl9mw8.fsf@arm.com> (raw)
In-Reply-To: <20231107103211.2837188-5-victor.donascimento@arm.com> (Victor Do Nascimento's message of "Tue, 7 Nov 2023 10:30:13 +0000")
Victor Do Nascimento <victor.donascimento@arm.com> writes:
> Implement the ACLE builtins for 128-bit system register manipulation:
>
> * __uint128_t __arm_rsr128(const char *special_register);
> * void __arm_wsr128(const char *special_register, __uint128_t value);
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64-builtins.cc (AARCH64_RSR128): New
> `enum aarch64_builtins' value.
> (AARCH64_WSR128): Likewise.
> (aarch64_init_rwsr_builtins): Init `__builtin_aarch64_rsr128'
> and `__builtin_aarch64_wsr128' builtins.
> (aarch64_expand_rwsr_builtin): Extend function to handle
> `__builtin_aarch64_{rsr|wsr}128'.
> * config/aarch64/aarch64-protos.h (aarch64_retrieve_sysreg):
> Update function signature.
> * config/aarch64/aarch64.cc (F_REG_128): New.
> (aarch64_retrieve_sysreg): Add 128-bit register mode check.
> * config/aarch64/aarch64.md (UNSPEC_SYSREG_RTI): New.
> (UNSPEC_SYSREG_WTI): Likewise.
> (aarch64_read_sysregti): Likewise.
> (aarch64_write_sysregti): Likewise.
> ---
> gcc/config/aarch64/aarch64-builtins.cc | 50 +++++++++++++++++++++-----
> gcc/config/aarch64/aarch64-protos.h | 2 +-
> gcc/config/aarch64/aarch64.cc | 6 +++-
> gcc/config/aarch64/aarch64.md | 18 ++++++++++
> gcc/config/aarch64/arm_acle.h | 11 ++++++
> 5 files changed, 77 insertions(+), 10 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc
> index c5f20f68bca..40d3788b5e0 100644
> --- a/gcc/config/aarch64/aarch64-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-builtins.cc
> @@ -815,11 +815,13 @@ enum aarch64_builtins
> AARCH64_RSR64,
> AARCH64_RSRF,
> AARCH64_RSRF64,
> + AARCH64_RSR128,
> AARCH64_WSR,
> AARCH64_WSRP,
> AARCH64_WSR64,
> AARCH64_WSRF,
> AARCH64_WSRF64,
> + AARCH64_WSR128,
> AARCH64_BUILTIN_MAX
> };
>
> @@ -1842,6 +1844,10 @@ aarch64_init_rwsr_builtins (void)
> = build_function_type_list (double_type_node, const_char_ptr_type, NULL);
> AARCH64_INIT_RWSR_BUILTINS_DECL (RSRF64, rsrf64, fntype);
>
> + fntype
> + = build_function_type_list (uint128_type_node, const_char_ptr_type, NULL);
> + AARCH64_INIT_RWSR_BUILTINS_DECL (RSR128, rsr128, fntype);
> +
> fntype
> = build_function_type_list (void_type_node, const_char_ptr_type,
> uint32_type_node, NULL);
> @@ -1867,6 +1873,12 @@ aarch64_init_rwsr_builtins (void)
> = build_function_type_list (void_type_node, const_char_ptr_type,
> double_type_node, NULL);
> AARCH64_INIT_RWSR_BUILTINS_DECL (WSRF64, wsrf64, fntype);
> +
> + fntype
> + = build_function_type_list (void_type_node, const_char_ptr_type,
> + uint128_type_node, NULL);
> + AARCH64_INIT_RWSR_BUILTINS_DECL (WSR128, wsr128, fntype);
> +
> }
>
> /* Initialize the memory tagging extension (MTE) builtins. */
> @@ -2710,6 +2722,7 @@ aarch64_expand_rwsr_builtin (tree exp, rtx target, int fcode)
> tree arg0, arg1;
> rtx const_str, input_val, subreg;
> enum machine_mode mode;
> + enum insn_code icode;
> class expand_operand ops[2];
>
> arg0 = CALL_EXPR_ARG (exp, 0);
> @@ -2718,7 +2731,18 @@ aarch64_expand_rwsr_builtin (tree exp, rtx target, int fcode)
> || fcode == AARCH64_WSRP
> || fcode == AARCH64_WSR64
> || fcode == AARCH64_WSRF
> - || fcode == AARCH64_WSRF64);
> + || fcode == AARCH64_WSRF64
> + || fcode == AARCH64_WSR128);
> +
> + bool op128 = (fcode == AARCH64_RSR128 || fcode == AARCH64_WSR128);
> + enum machine_mode sysreg_mode = op128 ? TImode : DImode;
> +
> + if (op128 && !TARGET_D128)
> + {
> + error_at (EXPR_LOCATION (exp), "128-bit system register suppport requires "
> + "the +d128 Armv9.4-A extension");
Elsewhere we've put feature names in quotes, since they're code
or code-adjacent. Probably also best to drop Armv9.4-A part,
since the requirement is tied only to +d128. So:
error_at (EXPR_LOCATION (exp),
"128-bit system register suppport requires the %<d128%>"
" extension");
(formatted that way to avoid a long line).
> + return const0_rtx;
> + }
>
> /* Argument 0 (system register name) must be a string literal. */
> gcc_assert (TREE_CODE (arg0) == ADDR_EXPR
> @@ -2741,7 +2765,7 @@ aarch64_expand_rwsr_builtin (tree exp, rtx target, int fcode)
> sysreg_name[pos] = TOLOWER (sysreg_name[pos]);
>
> const char* name_output = aarch64_retrieve_sysreg ((const char *) sysreg_name,
> - write_op);
> + write_op, op128);
> if (name_output == NULL)
> {
> error_at (EXPR_LOCATION (exp), "invalid system register name provided");
> @@ -2760,13 +2784,17 @@ aarch64_expand_rwsr_builtin (tree exp, rtx target, int fcode)
> mode = TYPE_MODE (TREE_TYPE (arg1));
> input_val = copy_to_mode_reg (mode, expand_normal (arg1));
>
> + icode = (op128 ? CODE_FOR_aarch64_write_sysregti
> + : CODE_FOR_aarch64_write_sysregdi);
> +
> switch (fcode)
> {
> case AARCH64_WSR:
> case AARCH64_WSRP:
> case AARCH64_WSR64:
> case AARCH64_WSRF64:
> - subreg = lowpart_subreg (DImode, input_val, mode);
> + case AARCH64_WSR128:
> + subreg = lowpart_subreg (sysreg_mode, input_val, mode);
> break;
> case AARCH64_WSRF:
> subreg = gen_lowpart_SUBREG (SImode, input_val);
> @@ -2775,8 +2803,8 @@ aarch64_expand_rwsr_builtin (tree exp, rtx target, int fcode)
> }
>
> create_fixed_operand (&ops[0], const_str);
> - create_input_operand (&ops[1], subreg, DImode);
> - expand_insn (CODE_FOR_aarch64_write_sysregdi, 2, ops);
> + create_input_operand (&ops[1], subreg, sysreg_mode);
> + expand_insn (icode, 2, ops);
>
> return target;
> }
> @@ -2784,10 +2812,13 @@ aarch64_expand_rwsr_builtin (tree exp, rtx target, int fcode)
> /* Read operations are implied by !write_op. */
> gcc_assert (call_expr_nargs (exp) == 1);
>
> + icode = (op128 ? CODE_FOR_aarch64_read_sysregti
> + : CODE_FOR_aarch64_read_sysregdi);
> +
> /* Emit the initial read_sysregdi rtx. */
> - create_output_operand (&ops[0], target, DImode);
> + create_output_operand (&ops[0], target, sysreg_mode);
> create_fixed_operand (&ops[1], const_str);
> - expand_insn (CODE_FOR_aarch64_read_sysregdi, 2, ops);
> + expand_insn (icode, 2, ops);
> target = ops[0].value;
>
> /* Do any necessary post-processing on the result. */
> @@ -2797,7 +2828,8 @@ aarch64_expand_rwsr_builtin (tree exp, rtx target, int fcode)
> case AARCH64_RSRP:
> case AARCH64_RSR64:
> case AARCH64_RSRF64:
> - return lowpart_subreg (TYPE_MODE (TREE_TYPE (exp)), target, DImode);
> + case AARCH64_RSR128:
> + return lowpart_subreg (TYPE_MODE (TREE_TYPE (exp)), target, sysreg_mode);
> case AARCH64_RSRF:
> subreg = gen_lowpart_SUBREG (SImode, target);
> return gen_lowpart_SUBREG (SFmode, subreg);
> @@ -3048,11 +3080,13 @@ aarch64_general_expand_builtin (unsigned int fcode, tree exp, rtx target,
> case AARCH64_RSR64:
> case AARCH64_RSRF:
> case AARCH64_RSRF64:
> + case AARCH64_RSR128:
> case AARCH64_WSR:
> case AARCH64_WSRP:
> case AARCH64_WSR64:
> case AARCH64_WSRF:
> case AARCH64_WSRF64:
> + case AARCH64_WSR128:
> return aarch64_expand_rwsr_builtin (exp, target, fcode);
> }
>
> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
> index dbd486cfea4..6a306134c2d 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -831,7 +831,7 @@ bool aarch64_sve_ptrue_svpattern_p (rtx, struct simd_immediate_info *);
> bool aarch64_simd_valid_immediate (rtx, struct simd_immediate_info *,
> enum simd_immediate_check w = AARCH64_CHECK_MOV);
> bool aarch64_valid_sysreg_name_p (const char *);
> -const char *aarch64_retrieve_sysreg (const char *, bool);
> +const char *aarch64_retrieve_sysreg (const char *, bool, bool);
> rtx aarch64_check_zero_based_sve_index_immediate (rtx);
> bool aarch64_sve_index_immediate_p (rtx);
> bool aarch64_sve_arith_immediate_p (machine_mode, rtx, bool);
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index c0d75f167be..ff7e75e1f19 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -2846,6 +2846,8 @@ typedef struct {
> #define F_ARCHEXT (1 << 4)
> /* Flag indicating register name is alias for another system register. */
> #define F_REG_ALIAS (1 << 5)
> +/* Flag indicatinig registers which may be implemented with 128-bits. */
> +#define F_REG_128 (1 << 6)
>
> /* Database of system registers, their encodings and architectural
> requirements. */
> @@ -28245,7 +28247,7 @@ aarch64_valid_sysreg_name_p (const char *regname)
> name, otherwise NULL. WRITE_P is true iff the register is being
> written to. */
> const char *
> -aarch64_retrieve_sysreg (const char *regname, bool write_p)
> +aarch64_retrieve_sysreg (const char *regname, bool write_p, bool is128op)
> {
> const sysreg_t *sysreg = aarch64_lookup_sysreg_map (regname);
> if (sysreg == NULL)
The comment should describe the new parameter.
Looks good otherwise.
Thanks,
Richard
> @@ -28255,6 +28257,8 @@ aarch64_retrieve_sysreg (const char *regname, bool write_p)
> else
> return NULL;
> }
> + if (is128op && !(sysreg->properties & F_REG_128))
> + return NULL;
> if ((write_p && (sysreg->properties & F_REG_READ))
> || (!write_p && (sysreg->properties & F_REG_WRITE)))
> return NULL;
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index aee8f8ad65a..3be813efcd4 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -282,7 +282,9 @@
> UNSPEC_RDFFR
> UNSPEC_WRFFR
> UNSPEC_SYSREG_RDI
> + UNSPEC_SYSREG_RTI
> UNSPEC_SYSREG_WDI
> + UNSPEC_SYSREG_WTI
> ;; Represents an SVE-style lane index, in which the indexing applies
> ;; within the containing 128-bit block.
> UNSPEC_SVE_LANE_SELECT
> @@ -486,6 +488,14 @@
> "mrs\t%x0, %1"
> )
>
> +(define_insn "aarch64_read_sysregti"
> + [(set (match_operand:TI 0 "register_operand" "=r")
> + (unspec_volatile:TI [(match_operand 1 "aarch64_sysreg_string" "")]
> + UNSPEC_SYSREG_RTI))]
> + "TARGET_D128"
> + "mrrs\t%x0, %H0, %x1"
> +)
> +
> (define_insn "aarch64_write_sysregdi"
> [(unspec_volatile:DI [(match_operand 0 "aarch64_sysreg_string" "")
> (match_operand:DI 1 "register_operand" "rZ")]
> @@ -494,6 +504,14 @@
> "msr\t%0, %x1"
> )
>
> +(define_insn "aarch64_write_sysregti"
> + [(unspec_volatile:TI [(match_operand 0 "aarch64_sysreg_string" "")
> + (match_operand:TI 1 "register_operand" "r")]
> + UNSPEC_SYSREG_WTI)]
> + "TARGET_D128"
> + "msrr\t%x0, %x1, %H1"
> +)
> +
> (define_insn "indirect_jump"
> [(set (pc) (match_operand:DI 0 "register_operand" "r"))]
> ""
> diff --git a/gcc/config/aarch64/arm_acle.h b/gcc/config/aarch64/arm_acle.h
> index 71ada878299..80282b361a4 100644
> --- a/gcc/config/aarch64/arm_acle.h
> +++ b/gcc/config/aarch64/arm_acle.h
> @@ -344,6 +344,17 @@ __rndrrs (uint64_t *__res)
> #define __arm_wsrf64(__regname, __value) \
> __builtin_aarch64_wsrf64 (__regname, __value)
>
> +#pragma GCC push_options
> +#pragma GCC target ("+nothing+d128")
> +
> +#define __arm_rsr128(__regname) \
> + __builtin_aarch64_rsr128 (__regname)
> +
> +#define __arm_wsr128(__regname, __value) \
> + __builtin_aarch64_wsr128 (__regname, __value)
> +
> +#pragma GCC pop_options
> +
> #ifdef __cplusplus
> }
> #endif
next prev parent reply other threads:[~2023-11-07 23:14 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-07 10:30 [PATCH 0/5] aarch64: Add Armv9.4-a 128-bit system-register read/write support Victor Do Nascimento
2023-11-07 10:30 ` [PATCH 1/5] aarch64: Add march flags for +the and +d128 arch extensions Victor Do Nascimento
2023-11-07 23:18 ` Richard Sandiford
2023-11-07 10:30 ` [PATCH 2/5] aarch64: Add support for GCS system registers with the +gcs modifier Victor Do Nascimento
2023-11-07 23:19 ` Richard Sandiford
2023-11-07 10:30 ` [PATCH 3/5] aarch64: Sync `aarch64-sys-regs.def' with Binutils Victor Do Nascimento
2023-11-07 23:17 ` Richard Sandiford
2023-11-07 10:30 ` [PATCH 4/5] aarch64: Implement 128-bit extension to ACLE sysreg r/w builtins Victor Do Nascimento
2023-11-07 23:14 ` Richard Sandiford [this message]
2023-11-07 10:30 ` [PATCH 5/5] aarch64: Add rsr128 and wsr128 ACLE tests Victor Do Nascimento
2023-11-07 22:51 ` Richard Sandiford
2023-11-08 9:49 ` Christophe Lyon
2023-11-08 10:03 ` Richard Sandiford
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mptcywl9mw8.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=Richard.Earnshaw@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=kyrylo.tkachov@arm.com \
--cc=victor.donascimento@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).