[v2] aarch64: Support for FEAT_PCDPHINT
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
fail
|
Test failed
|
Commit Message
From: Ezra Sitorus <ezra.sitorus@arm.com>
FEAT_PCDPHINT - Producer-consumer data placement hints - is an optional
ISA extension that provides hint instructions to indicate:
- a store in the current execution thread is generating data at a specific
location, which a thread of execution on one or more other observers is
waiting on.
- the thread of execution on the current PE will read a location that may not
yet have been written with the value to be consumed.
This extension introduces:
- STSHH, a hint instruction, with operands (policies) keep and strm
- PRFM *IR*, a new prefetch memory operand.
Details of FEAT_PCPHINT can be found here:
https://developer.arm.com/documentation/109697/2024_12/Feature-descriptions/The-Armv9-6-architecture-extension
https://developer.arm.com/documentation/ddi0602/2024-12/Base-Instructions/STSHH--Store-shared-hint-?lang=en
https://developer.arm.com/documentation/ddi0602/2024-12/Base-Instructions/PRFM--immediate---Prefetch-memory--immediate--?lang=en
---
Addressed the issues from v1. Could someone please commit this if it looks ok?
gas/config/tc-aarch64.c | 9 ++++++++-
gas/testsuite/gas/aarch64/pcdphint-bad.d | 4 ++++
gas/testsuite/gas/aarch64/pcdphint-bad.l | 5 +++++
gas/testsuite/gas/aarch64/pcdphint-bad.s | 5 +++++
gas/testsuite/gas/aarch64/stshh.d | 10 ++++++++++
gas/testsuite/gas/aarch64/stshh.s | 6 ++++++
gas/testsuite/gas/aarch64/system.d | 8 ++++----
include/opcode/aarch64.h | 3 +++
opcodes/aarch64-opc.c | 8 +++++++-
opcodes/aarch64-tbl.h | 3 +++
10 files changed, 55 insertions(+), 6 deletions(-)
create mode 100644 gas/testsuite/gas/aarch64/pcdphint-bad.d
create mode 100644 gas/testsuite/gas/aarch64/pcdphint-bad.l
create mode 100644 gas/testsuite/gas/aarch64/pcdphint-bad.s
create mode 100644 gas/testsuite/gas/aarch64/stshh.d
create mode 100644 gas/testsuite/gas/aarch64/stshh.s
Comments
Thanks - I've left a few comments below.
On Thu, Apr 10, 2025 at 04:29:53PM +0100, Ezra.Sitorus@arm.com wrote:
> From: Ezra Sitorus <ezra.sitorus@arm.com>
>
> FEAT_PCDPHINT - Producer-consumer data placement hints - is an optional
> ISA extension that provides hint instructions to indicate:
> - a store in the current execution thread is generating data at a specific
> location, which a thread of execution on one or more other observers is
> waiting on.
> - the thread of execution on the current PE will read a location that may not
> yet have been written with the value to be consumed.
>
> This extension introduces:
> - STSHH, a hint instruction, with operands (policies) keep and strm
> - PRFM *IR*, a new prefetch memory operand.
>
> Details of FEAT_PCPHINT can be found here:
> https://developer.arm.com/documentation/109697/2024_12/Feature-descriptions/The-Armv9-6-architecture-extension
> https://developer.arm.com/documentation/ddi0602/2024-12/Base-Instructions/STSHH--Store-shared-hint-?lang=en
> https://developer.arm.com/documentation/ddi0602/2024-12/Base-Instructions/PRFM--immediate---Prefetch-memory--immediate--?lang=en
As in your other patch, I think parts of this commit message are unnecessarily
verbose.
> ---
> Addressed the issues from v1. Could someone please commit this if it looks ok?
>
> gas/config/tc-aarch64.c | 9 ++++++++-
> gas/testsuite/gas/aarch64/pcdphint-bad.d | 4 ++++
> gas/testsuite/gas/aarch64/pcdphint-bad.l | 5 +++++
> gas/testsuite/gas/aarch64/pcdphint-bad.s | 5 +++++
> gas/testsuite/gas/aarch64/stshh.d | 10 ++++++++++
> gas/testsuite/gas/aarch64/stshh.s | 6 ++++++
> gas/testsuite/gas/aarch64/system.d | 8 ++++----
> include/opcode/aarch64.h | 3 +++
> opcodes/aarch64-opc.c | 8 +++++++-
> opcodes/aarch64-tbl.h | 3 +++
> 10 files changed, 55 insertions(+), 6 deletions(-)
> create mode 100644 gas/testsuite/gas/aarch64/pcdphint-bad.d
> create mode 100644 gas/testsuite/gas/aarch64/pcdphint-bad.l
> create mode 100644 gas/testsuite/gas/aarch64/pcdphint-bad.s
> create mode 100644 gas/testsuite/gas/aarch64/stshh.d
> create mode 100644 gas/testsuite/gas/aarch64/stshh.s
>
> diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
> index e071ad11f46..c689c7dcbb4 100644
> --- a/gas/config/tc-aarch64.c
> +++ b/gas/config/tc-aarch64.c
> @@ -4607,7 +4607,9 @@ parse_hint_opt (const char *name, char **str,
> && o->value != HINT_OPD_CSYNC)
> || ((strcmp ("bti", name) == 0)
> && (o->value != HINT_OPD_C && o->value != HINT_OPD_J
> - && o->value != HINT_OPD_JC)))
> + && o->value != HINT_OPD_JC))
> + || ((strcmp ("stshh", name) == 0)
> + && (o->value != HINT_OPD_KEEP && o->value != HINT_OPD_STRM)))
> return false;
I don't like this extensive if statement. This is an existing issue so would
be a separate clean up, but I think the check could be improved moving it into
the caller and doing a comparison with the instruction's mask and opcode
(something like imm<<5 & mask == value & mask & 0xfe0).
>
> *str = q;
> @@ -8150,6 +8152,11 @@ parse_operands (char *str, const aarch64_opcode *opcode)
> goto failure;
> break;
>
> + case AARCH64_OPND_STSHH_POLICY:
> + if (!parse_hint_opt (opcode->name, &str, &(info->hint_option)))
> + goto failure;
> + break;
> +
This and the existing three hint cases have identical bodies, so they can be
combined (again, this could be a separate clean up).
> case AARCH64_OPND_SME_ZAda_1b:
> case AARCH64_OPND_SME_ZAda_2b:
> case AARCH64_OPND_SME_ZAda_3b:
> diff --git a/gas/testsuite/gas/aarch64/pcdphint-bad.d b/gas/testsuite/gas/aarch64/pcdphint-bad.d
> new file mode 100644
> index 00000000000..33373b45356
> --- /dev/null
> +++ b/gas/testsuite/gas/aarch64/pcdphint-bad.d
> @@ -0,0 +1,4 @@
> +#name: Negative test of PCPHINT instructions.
> +#as: -march=armv8-a
> +#source: pcdphint-bad.s
> +#error_output: pcdphint-bad.l
> diff --git a/gas/testsuite/gas/aarch64/pcdphint-bad.l b/gas/testsuite/gas/aarch64/pcdphint-bad.l
> new file mode 100644
> index 00000000000..ae542f852d5
> --- /dev/null
> +++ b/gas/testsuite/gas/aarch64/pcdphint-bad.l
> @@ -0,0 +1,5 @@
> +[^ :]+: Assembler messages:
> +[^ :]+:[0-9]+: Error: operand 1 must be STSHH policies keep/strm -- `stshh kee'
> +[^ :]+:[0-9]+: Error: operand 1 must be STSHH policies keep/strm -- `stshh strmm'
> +[^ :]+:[0-9]+: Error: operand 1 must be STSHH policies keep/strm -- `stshh'
> +[^ :]+:[0-9]+: Error: constant expression required at operand 1 -- `prfm ir1234'
> diff --git a/gas/testsuite/gas/aarch64/pcdphint-bad.s b/gas/testsuite/gas/aarch64/pcdphint-bad.s
> new file mode 100644
> index 00000000000..18967882f56
> --- /dev/null
> +++ b/gas/testsuite/gas/aarch64/pcdphint-bad.s
> @@ -0,0 +1,5 @@
> + stshh kee
> + stshh strmm
> + stshh
> +
> + prfm ir1234
> diff --git a/gas/testsuite/gas/aarch64/stshh.d b/gas/testsuite/gas/aarch64/stshh.d
> new file mode 100644
> index 00000000000..095048d143f
> --- /dev/null
> +++ b/gas/testsuite/gas/aarch64/stshh.d
> @@ -0,0 +1,10 @@
> +#as: -march=armv8-a
> +#objdump: -dr
> +
> +.*: file format .*
> +
> +Disassembly of section \.text:
> +
> +0+ <.*>:
> +.*: d503261f stshh keep
> +.*: d503263f stshh strm
> diff --git a/gas/testsuite/gas/aarch64/stshh.s b/gas/testsuite/gas/aarch64/stshh.s
> new file mode 100644
> index 00000000000..01632aaf532
> --- /dev/null
> +++ b/gas/testsuite/gas/aarch64/stshh.s
> @@ -0,0 +1,6 @@
> +// Test file for AArch64 stshh.
> +
> + .text
> +
> + stshh keep
> + stshh strm
> diff --git a/gas/testsuite/gas/aarch64/system.d b/gas/testsuite/gas/aarch64/system.d
> index c1400771d43..907acdc549d 100644
> --- a/gas/testsuite/gas/aarch64/system.d
> +++ b/gas/testsuite/gas/aarch64/system.d
> @@ -63,8 +63,8 @@ Disassembly of section \.text:
> .*: d50325bf hint #0x2d
> .*: d50325df hint #0x2e
> .*: d50325ff hint #0x2f
> -.*: d503261f hint #0x30
> -.*: d503263f hint #0x31
> +.*: d503261f (hint #0x30|stshh keep)
> +.*: d503263f (hint #0x31|stshh strm)
> .*: d503265f hint #0x32
> .*: d503267f hint #0x33
> .*: d503269f hint #0x34
> @@ -330,9 +330,9 @@ Disassembly of section \.text:
> .*: f8af6bf7 prfm pstslcstrm, \[sp, x15\]
> .*: f8be58f7 prfm pstslcstrm, \[x7, w30, uxtw #3\]
> .*: f9800c77 prfm pstslcstrm, \[x3, #24\]
> -.*: d8000018 prfm #0x18, 0 <LABEL1>
> +.*: d8000018 prfm ir, 0 <LABEL1>
PRFM (immediate) doesn't support type IR, so this line shouldn't have changed.
I think you've also mistakenly added IR support to PRFUM. Both of these (and
PRFM (register)) should have tests that we reject the IR type, although I
actually already have a test for prfum #18 in a patch I'm currently working on.
> .*: R_AARCH64_(P32_|)LD_PREL_LO19 LABEL1
> -.*: f9800c78 prfm #0x18, \[x3, #24\]
> +.*: f9800c78 prfm ir, \[x3, #24\]
> .*: d8000019 prfm #0x19, 0 <LABEL1>
> .*: R_AARCH64_(P32_|)LD_PREL_LO19 LABEL1
> .*: f9800c79 prfm #0x19, \[x3, #24\]
> diff --git a/include/opcode/aarch64.h b/include/opcode/aarch64.h
> index dfe3f05820a..d5e1edcee3e 100644
> --- a/include/opcode/aarch64.h
> +++ b/include/opcode/aarch64.h
> @@ -688,6 +688,7 @@ enum aarch64_opnd
> AARCH64_OPND_BARRIER_PSB, /* Barrier operand for PSB. */
> AARCH64_OPND_BARRIER_GCSB, /* Barrier operand for GCSB. */
> AARCH64_OPND_BTI_TARGET, /* BTI {<target>}. */
> + AARCH64_OPND_STSHH_POLICY, /* STSHH {<policy>}. */
> AARCH64_OPND_BRBOP, /* BRB operation IALL or INJ in bit 5. */
> AARCH64_OPND_Rt_IN_SYS_ALIASES, /* Defaulted and omitted Rt used in SYS aliases such as brb. */
> AARCH64_OPND_LSE128_Rt, /* LSE128 <Xt1>. */
> @@ -1770,6 +1771,8 @@ struct aarch64_inst
> #define HINT_OPD_C 0x22
> #define HINT_OPD_J 0x24
> #define HINT_OPD_JC 0x26
> +#define HINT_OPD_KEEP 0x30
> +#define HINT_OPD_STRM 0x31
> #define HINT_OPD_NULL 0x00
>
>
> diff --git a/opcodes/aarch64-opc.c b/opcodes/aarch64-opc.c
> index 4f0c71696fa..de2321987cf 100644
> --- a/opcodes/aarch64-opc.c
> +++ b/opcodes/aarch64-opc.c
> @@ -596,6 +596,8 @@ const struct aarch64_name_value_pair aarch64_hint_options[] =
> { "c", HINT_OPD_C }, /* BTI C. */
> { "j", HINT_OPD_J }, /* BTI J. */
> { "jc", HINT_OPD_JC }, /* BTI JC. */
> + { "keep", HINT_OPD_KEEP }, /* STSHH KEEP */
> + { "strm", HINT_OPD_STRM }, /* STSHH STRM */
> { NULL, HINT_OPD_NULL },
> };
>
> @@ -629,7 +631,7 @@ const struct aarch64_name_value_pair aarch64_prfops[32] =
> { "pstl3strm", B(2, 3, 1) },
> { "pstslckeep", B(2, 4, 0) },
> { "pstslcstrm", B(2, 4, 1) },
> - { NULL, 0x18 },
> + { "ir", 0x18 },
> { NULL, 0x19 },
> { NULL, 0x1a },
> { NULL, 0x1b },
> @@ -5071,6 +5073,10 @@ aarch64_print_operand (char *buf, size_t size, bfd_vma pc,
> style_sub_mnem (styler, opnd->hint_option->name));
> break;
>
> + case AARCH64_OPND_STSHH_POLICY:
> + snprintf (buf, size, "%s", style_sub_mnem (styler, opnd->hint_option->name));
> + break;
> +
> case AARCH64_OPND_MOPS_ADDR_Rd:
> case AARCH64_OPND_MOPS_ADDR_Rs:
> snprintf (buf, size, "[%s]!",
> diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h
> index 8b64eb07067..38002486943 100644
> --- a/opcodes/aarch64-tbl.h
> +++ b/opcodes/aarch64-tbl.h
> @@ -4588,6 +4588,7 @@ const struct aarch64_opcode aarch64_opcode_table[] =
> SME_INSN ("smstop", 0xd503407f, 0xfffff1ff, sme_stop, 0, OP1 (SME_SM_ZA), {}, F_SYS_WRITE, 0),
> /* System. */
> CHK_INSN ("chkfeat", 0xd503251f, 0xffffffff, OP1 (X16), QL_I1X, 0),
> + CORE_INSN ("stshh", 0xd503261f, 0xffffff1f, ic_system, 0, OP1 (STSHH_POLICY), {}, F_ALIAS),
This mask should be 0xffffffdf.
> CORE_INSN ("msr", 0xd500401f, 0xfff8f01f, ic_system, 0, OP2 (PSTATEFIELD, UIMM4), {}, F_SYS_WRITE),
> CORE_INSN ("hint",0xd503201f, 0xfffff01f, ic_system, 0, OP1 (UIMM7), {}, F_HAS_ALIAS),
> CORE_INSN ("nop", 0xd503201f, 0xffffffff, ic_system, 0, OP0 (), {}, F_ALIAS),
> @@ -7235,6 +7236,8 @@ const struct aarch64_opcode aarch64_opcode_table[] =
> "the GCSB option name DSYNC") \
> Y(SYSTEM, hint, "BTI_TARGET", 0, F (), \
> "BTI targets j/c/jc") \
> + Y(SYSTEM, hint, "STSHH_POLICY", 0, F(), \
> + "STSHH policies keep/strm") \
> Y(SYSTEM, imm, "BRBOP", 0, F(FLD_brbop), \
> "Branch Record Buffer operation operand") \
> Y(INT_REG, regno, "Rt_IN_SYS_ALIASES", 0, F(FLD_Rt), \
> --
> 2.45.2
>
@@ -4607,7 +4607,9 @@ parse_hint_opt (const char *name, char **str,
&& o->value != HINT_OPD_CSYNC)
|| ((strcmp ("bti", name) == 0)
&& (o->value != HINT_OPD_C && o->value != HINT_OPD_J
- && o->value != HINT_OPD_JC)))
+ && o->value != HINT_OPD_JC))
+ || ((strcmp ("stshh", name) == 0)
+ && (o->value != HINT_OPD_KEEP && o->value != HINT_OPD_STRM)))
return false;
*str = q;
@@ -8150,6 +8152,11 @@ parse_operands (char *str, const aarch64_opcode *opcode)
goto failure;
break;
+ case AARCH64_OPND_STSHH_POLICY:
+ if (!parse_hint_opt (opcode->name, &str, &(info->hint_option)))
+ goto failure;
+ break;
+
case AARCH64_OPND_SME_ZAda_1b:
case AARCH64_OPND_SME_ZAda_2b:
case AARCH64_OPND_SME_ZAda_3b:
new file mode 100644
@@ -0,0 +1,4 @@
+#name: Negative test of PCPHINT instructions.
+#as: -march=armv8-a
+#source: pcdphint-bad.s
+#error_output: pcdphint-bad.l
new file mode 100644
@@ -0,0 +1,5 @@
+[^ :]+: Assembler messages:
+[^ :]+:[0-9]+: Error: operand 1 must be STSHH policies keep/strm -- `stshh kee'
+[^ :]+:[0-9]+: Error: operand 1 must be STSHH policies keep/strm -- `stshh strmm'
+[^ :]+:[0-9]+: Error: operand 1 must be STSHH policies keep/strm -- `stshh'
+[^ :]+:[0-9]+: Error: constant expression required at operand 1 -- `prfm ir1234'
new file mode 100644
@@ -0,0 +1,5 @@
+ stshh kee
+ stshh strmm
+ stshh
+
+ prfm ir1234
new file mode 100644
@@ -0,0 +1,10 @@
+#as: -march=armv8-a
+#objdump: -dr
+
+.*: file format .*
+
+Disassembly of section \.text:
+
+0+ <.*>:
+.*: d503261f stshh keep
+.*: d503263f stshh strm
new file mode 100644
@@ -0,0 +1,6 @@
+// Test file for AArch64 stshh.
+
+ .text
+
+ stshh keep
+ stshh strm
@@ -63,8 +63,8 @@ Disassembly of section \.text:
.*: d50325bf hint #0x2d
.*: d50325df hint #0x2e
.*: d50325ff hint #0x2f
-.*: d503261f hint #0x30
-.*: d503263f hint #0x31
+.*: d503261f (hint #0x30|stshh keep)
+.*: d503263f (hint #0x31|stshh strm)
.*: d503265f hint #0x32
.*: d503267f hint #0x33
.*: d503269f hint #0x34
@@ -330,9 +330,9 @@ Disassembly of section \.text:
.*: f8af6bf7 prfm pstslcstrm, \[sp, x15\]
.*: f8be58f7 prfm pstslcstrm, \[x7, w30, uxtw #3\]
.*: f9800c77 prfm pstslcstrm, \[x3, #24\]
-.*: d8000018 prfm #0x18, 0 <LABEL1>
+.*: d8000018 prfm ir, 0 <LABEL1>
.*: R_AARCH64_(P32_|)LD_PREL_LO19 LABEL1
-.*: f9800c78 prfm #0x18, \[x3, #24\]
+.*: f9800c78 prfm ir, \[x3, #24\]
.*: d8000019 prfm #0x19, 0 <LABEL1>
.*: R_AARCH64_(P32_|)LD_PREL_LO19 LABEL1
.*: f9800c79 prfm #0x19, \[x3, #24\]
@@ -688,6 +688,7 @@ enum aarch64_opnd
AARCH64_OPND_BARRIER_PSB, /* Barrier operand for PSB. */
AARCH64_OPND_BARRIER_GCSB, /* Barrier operand for GCSB. */
AARCH64_OPND_BTI_TARGET, /* BTI {<target>}. */
+ AARCH64_OPND_STSHH_POLICY, /* STSHH {<policy>}. */
AARCH64_OPND_BRBOP, /* BRB operation IALL or INJ in bit 5. */
AARCH64_OPND_Rt_IN_SYS_ALIASES, /* Defaulted and omitted Rt used in SYS aliases such as brb. */
AARCH64_OPND_LSE128_Rt, /* LSE128 <Xt1>. */
@@ -1770,6 +1771,8 @@ struct aarch64_inst
#define HINT_OPD_C 0x22
#define HINT_OPD_J 0x24
#define HINT_OPD_JC 0x26
+#define HINT_OPD_KEEP 0x30
+#define HINT_OPD_STRM 0x31
#define HINT_OPD_NULL 0x00
@@ -596,6 +596,8 @@ const struct aarch64_name_value_pair aarch64_hint_options[] =
{ "c", HINT_OPD_C }, /* BTI C. */
{ "j", HINT_OPD_J }, /* BTI J. */
{ "jc", HINT_OPD_JC }, /* BTI JC. */
+ { "keep", HINT_OPD_KEEP }, /* STSHH KEEP */
+ { "strm", HINT_OPD_STRM }, /* STSHH STRM */
{ NULL, HINT_OPD_NULL },
};
@@ -629,7 +631,7 @@ const struct aarch64_name_value_pair aarch64_prfops[32] =
{ "pstl3strm", B(2, 3, 1) },
{ "pstslckeep", B(2, 4, 0) },
{ "pstslcstrm", B(2, 4, 1) },
- { NULL, 0x18 },
+ { "ir", 0x18 },
{ NULL, 0x19 },
{ NULL, 0x1a },
{ NULL, 0x1b },
@@ -5071,6 +5073,10 @@ aarch64_print_operand (char *buf, size_t size, bfd_vma pc,
style_sub_mnem (styler, opnd->hint_option->name));
break;
+ case AARCH64_OPND_STSHH_POLICY:
+ snprintf (buf, size, "%s", style_sub_mnem (styler, opnd->hint_option->name));
+ break;
+
case AARCH64_OPND_MOPS_ADDR_Rd:
case AARCH64_OPND_MOPS_ADDR_Rs:
snprintf (buf, size, "[%s]!",
@@ -4588,6 +4588,7 @@ const struct aarch64_opcode aarch64_opcode_table[] =
SME_INSN ("smstop", 0xd503407f, 0xfffff1ff, sme_stop, 0, OP1 (SME_SM_ZA), {}, F_SYS_WRITE, 0),
/* System. */
CHK_INSN ("chkfeat", 0xd503251f, 0xffffffff, OP1 (X16), QL_I1X, 0),
+ CORE_INSN ("stshh", 0xd503261f, 0xffffff1f, ic_system, 0, OP1 (STSHH_POLICY), {}, F_ALIAS),
CORE_INSN ("msr", 0xd500401f, 0xfff8f01f, ic_system, 0, OP2 (PSTATEFIELD, UIMM4), {}, F_SYS_WRITE),
CORE_INSN ("hint",0xd503201f, 0xfffff01f, ic_system, 0, OP1 (UIMM7), {}, F_HAS_ALIAS),
CORE_INSN ("nop", 0xd503201f, 0xffffffff, ic_system, 0, OP0 (), {}, F_ALIAS),
@@ -7235,6 +7236,8 @@ const struct aarch64_opcode aarch64_opcode_table[] =
"the GCSB option name DSYNC") \
Y(SYSTEM, hint, "BTI_TARGET", 0, F (), \
"BTI targets j/c/jc") \
+ Y(SYSTEM, hint, "STSHH_POLICY", 0, F(), \
+ "STSHH policies keep/strm") \
Y(SYSTEM, imm, "BRBOP", 0, F(FLD_brbop), \
"Branch Record Buffer operation operand") \
Y(INT_REG, regno, "Rt_IN_SYS_ALIASES", 0, F(FLD_Rt), \