| Message ID | fd916637-84a4-7e48-83d5-4f412df37233@e124511.cambridge.arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: Fix intrinsic availability [PR112108] | expand |
Andrew Carlotti <andrew.carlotti@arm.com> writes: > The availability of tme intrinsics was previously gated at both > initialisation time (using global target options) and usage time > (accounting for function-specific target options). This patch removes > the check at initialisation time, and also moves the intrinsics out of > the header file to allow for better error messages (matching the > existing error messages for SVE intrinsics). > > gcc/ChangeLog: > > PR target/112108 > * config/aarch64/aarch64-builtins.cc (aarch64_init_tme_builtins): > (aarch64_general_init_builtins): Move tme initialisation... > (handle_arm_acle_h): ...to here, and remove feature check. > (aarch64_general_check_builtin_call): Check tme intrinsics. > (aarch64_expand_builtin_tme): Check feature availability. > * config/aarch64/arm_acle.h (__tstart, __tcommit, __tcancel) > (__ttest): Remove. > (_TMFAILURE_*): Define unconditionally. > > gcc/testsuite/ChangeLog: > > PR target/112108 > * gcc.target/aarch64/acle/tme_guard-1.c: New test. > * gcc.target/aarch64/acle/tme_guard-2.c: New test. > * gcc.target/aarch64/acle/tme_guard-3.c: New test. > * gcc.target/aarch64/acle/tme_guard-4.c: New test. > > > diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc > index d0fb8bc1d1fedb382cba1a1f09a9c3ce6757ee22..f7d31d8c4308b4a883f8ce7df5c3ee319abbbb9c 100644 > --- a/gcc/config/aarch64/aarch64-builtins.cc > +++ b/gcc/config/aarch64/aarch64-builtins.cc > @@ -1791,19 +1791,19 @@ aarch64_init_tme_builtins (void) > = build_function_type_list (void_type_node, uint64_type_node, NULL); > > aarch64_builtin_decls[AARCH64_TME_BUILTIN_TSTART] > - = aarch64_general_add_builtin ("__builtin_aarch64_tstart", > + = aarch64_general_simulate_builtin ("__tstart", > ftype_uint64_void, > AARCH64_TME_BUILTIN_TSTART); > aarch64_builtin_decls[AARCH64_TME_BUILTIN_TTEST] > - = aarch64_general_add_builtin ("__builtin_aarch64_ttest", > + = aarch64_general_simulate_builtin ("__ttest", > ftype_uint64_void, > AARCH64_TME_BUILTIN_TTEST); > aarch64_builtin_decls[AARCH64_TME_BUILTIN_TCOMMIT] > - = aarch64_general_add_builtin ("__builtin_aarch64_tcommit", > + = aarch64_general_simulate_builtin ("__tcommit", > ftype_void_void, > AARCH64_TME_BUILTIN_TCOMMIT); > aarch64_builtin_decls[AARCH64_TME_BUILTIN_TCANCEL] > - = aarch64_general_add_builtin ("__builtin_aarch64_tcancel", > + = aarch64_general_simulate_builtin ("__tcancel", > ftype_void_uint64, > AARCH64_TME_BUILTIN_TCANCEL); Very minor, sorry, but could you reindent the arguments to match the new function name? > } > @@ -2068,6 +2068,7 @@ handle_arm_acle_h (void) > { > if (TARGET_LS64) > aarch64_init_ls64_builtins (); > + aarch64_init_tme_builtins (); > } > > /* Initialize fpsr fpcr getters and setters. */ > @@ -2160,9 +2161,6 @@ aarch64_general_init_builtins (void) > if (!TARGET_ILP32) > aarch64_init_pauth_hint_builtins (); > > - if (TARGET_TME) > - aarch64_init_tme_builtins (); > - > if (TARGET_MEMTAG) > aarch64_init_memtag_builtins (); > > @@ -2289,6 +2287,7 @@ aarch64_general_check_builtin_call (location_t location, vec<location_t>, > unsigned int code, tree fndecl, > unsigned int nargs ATTRIBUTE_UNUSED, tree *args) > { > + tree decl = aarch64_builtin_decls[code]; > switch (code) > { > case AARCH64_RSR: > @@ -2301,15 +2300,28 @@ aarch64_general_check_builtin_call (location_t location, vec<location_t>, > case AARCH64_WSR64: > case AARCH64_WSRF: > case AARCH64_WSRF64: > - tree addr = STRIP_NOPS (args[0]); > - if (TREE_CODE (TREE_TYPE (addr)) != POINTER_TYPE > - || TREE_CODE (addr) != ADDR_EXPR > - || TREE_CODE (TREE_OPERAND (addr, 0)) != STRING_CST) > - { > - error_at (location, "first argument to %qD must be a string literal", > - fndecl); > - return false; > - } > + { > + tree addr = STRIP_NOPS (args[0]); > + if (TREE_CODE (TREE_TYPE (addr)) != POINTER_TYPE > + || TREE_CODE (addr) != ADDR_EXPR > + || TREE_CODE (TREE_OPERAND (addr, 0)) != STRING_CST) > + { > + error_at (location, "first argument to %qD must be a string literal", > + fndecl); > + return false; > + } > + break; > + } > + > + case AARCH64_TME_BUILTIN_TSTART: > + case AARCH64_TME_BUILTIN_TCOMMIT: > + case AARCH64_TME_BUILTIN_TTEST: > + case AARCH64_TME_BUILTIN_TCANCEL: > + return aarch64_check_required_extensions (location, decl, > + AARCH64_FL_TME, false); > + > + default: > + break; > } > /* Default behavior. */ > return true; > @@ -2734,6 +2746,11 @@ aarch64_expand_fcmla_builtin (tree exp, rtx target, int fcode) > static rtx > aarch64_expand_builtin_tme (int fcode, tree exp, rtx target) > { > + tree fndecl = aarch64_builtin_decls[fcode]; > + if (!aarch64_check_required_extensions (EXPR_LOCATION (exp), fndecl, > + AARCH64_FL_TME, false)) > + return target; > + Does this ever trigger? The SVE code also checks during expansion, but I think that's because of the manual overload resolution. Perhaps I misremember though... LGTM otherwise, but please leave 24 hours for others to comment. Thanks, Richard > switch (fcode) > { > case AARCH64_TME_BUILTIN_TSTART: > diff --git a/gcc/config/aarch64/arm_acle.h b/gcc/config/aarch64/arm_acle.h > index 2aa681090fa205449cf1ac63151565f960716189..2d84ab1bd3f3241196727d7a632a155014708081 100644 > --- a/gcc/config/aarch64/arm_acle.h > +++ b/gcc/config/aarch64/arm_acle.h > @@ -252,10 +252,7 @@ __crc32d (uint32_t __a, uint64_t __b) > > #pragma GCC pop_options > > -#ifdef __ARM_FEATURE_TME > -#pragma GCC push_options > -#pragma GCC target ("+nothing+tme") > - > +/* Constants for TME failure reason. */ > #define _TMFAILURE_REASON 0x00007fffu > #define _TMFAILURE_RTRY 0x00008000u > #define _TMFAILURE_CNCL 0x00010000u > @@ -268,37 +265,6 @@ __crc32d (uint32_t __a, uint64_t __b) > #define _TMFAILURE_INT 0x00800000u > #define _TMFAILURE_TRIVIAL 0x01000000u > > -__extension__ extern __inline uint64_t > -__attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > -__tstart (void) > -{ > - return __builtin_aarch64_tstart (); > -} > - > -__extension__ extern __inline void > -__attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > -__tcommit (void) > -{ > - __builtin_aarch64_tcommit (); > -} > - > -__extension__ extern __inline void > -__attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > -__tcancel (const uint64_t __reason) > -{ > - __builtin_aarch64_tcancel (__reason); > -} > - > -__extension__ extern __inline uint64_t > -__attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > -__ttest (void) > -{ > - return __builtin_aarch64_ttest (); > -} > - > -#pragma GCC pop_options > -#endif > - > #ifdef __ARM_FEATURE_LS64 > typedef __arm_data512_t data512_t; > #endif > diff --git a/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-1.c b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-1.c > new file mode 100644 > index 0000000000000000000000000000000000000000..9894d3341f6bc352c22ad95d4d1e000207ca8d00 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-1.c > @@ -0,0 +1,9 @@ > +/* { dg-do compile } */ > +/* { dg-additional-options "-march=armv8-a" } */ > + > +#include <arm_acle.h> > + > +void foo (void) > +{ > + __tcommit (); /* { dg-error {ACLE function '__tcommit' requires ISA extension 'tme'} } */ > +} > diff --git a/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-2.c b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-2.c > new file mode 100644 > index 0000000000000000000000000000000000000000..4e3d69712b14a8123f45a2ead02b5048883614d9 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-2.c > @@ -0,0 +1,10 @@ > +/* { dg-do compile } */ > +/* { dg-additional-options "-march=armv8-a" } */ > + > +#include <arm_acle.h> > + > +#pragma GCC target("arch=armv8-a+tme") > +void foo (void) > +{ > + __tcommit (); > +} > diff --git a/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-3.c b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-3.c > new file mode 100644 > index 0000000000000000000000000000000000000000..5f480ebb8209fdaeb4baa77dbdf5467d16936644 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-3.c > @@ -0,0 +1,9 @@ > +/* { dg-do compile } */ > +/* { dg-additional-options "-march=armv8-a+tme -mgeneral-regs-only" } */ > + > +#include <arm_acle.h> > + > +void foo (void) > +{ > + __tcommit (); > +} > diff --git a/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-4.c b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-4.c > new file mode 100644 > index 0000000000000000000000000000000000000000..bf4d368370c614ffe33035d9ec4f86988f3f1c30 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-4.c > @@ -0,0 +1,10 @@ > +/* { dg-do compile } */ > +/* { dg-additional-options "-march=armv8-a+tme" } */ > + > +#include <arm_acle.h> > + > +#pragma GCC target("arch=armv8-a") > +void foo (void) > +{ > + __tcommit (); /* { dg-error {ACLE function '__tcommit' requires ISA extension 'tme'} } */ > +}
On Thu, Aug 08, 2024 at 04:48:38PM +0100, Richard Sandiford wrote: > Andrew Carlotti <andrew.carlotti@arm.com> writes: > > The availability of tme intrinsics was previously gated at both > > initialisation time (using global target options) and usage time > > (accounting for function-specific target options). This patch removes > > the check at initialisation time, and also moves the intrinsics out of > > the header file to allow for better error messages (matching the > > existing error messages for SVE intrinsics). > > > > gcc/ChangeLog: > > > > PR target/112108 > > * config/aarch64/aarch64-builtins.cc (aarch64_init_tme_builtins): > > (aarch64_general_init_builtins): Move tme initialisation... > > (handle_arm_acle_h): ...to here, and remove feature check. > > (aarch64_general_check_builtin_call): Check tme intrinsics. > > (aarch64_expand_builtin_tme): Check feature availability. > > * config/aarch64/arm_acle.h (__tstart, __tcommit, __tcancel) > > (__ttest): Remove. > > (_TMFAILURE_*): Define unconditionally. > > > > gcc/testsuite/ChangeLog: > > > > PR target/112108 > > * gcc.target/aarch64/acle/tme_guard-1.c: New test. > > * gcc.target/aarch64/acle/tme_guard-2.c: New test. > > * gcc.target/aarch64/acle/tme_guard-3.c: New test. > > * gcc.target/aarch64/acle/tme_guard-4.c: New test. > > > > ... > > @@ -2734,6 +2746,11 @@ aarch64_expand_fcmla_builtin (tree exp, rtx target, int fcode) > > static rtx > > aarch64_expand_builtin_tme (int fcode, tree exp, rtx target) > > { > > + tree fndecl = aarch64_builtin_decls[fcode]; > > + if (!aarch64_check_required_extensions (EXPR_LOCATION (exp), fndecl, > > + AARCH64_FL_TME, false)) > > + return target; > > + > > Does this ever trigger? The SVE code also checks during expansion, > but I think that's because of the manual overload resolution. Perhaps > I misremember though... I don't see any reason for them to trigger; I only included them because they were present in the SVE code and they shouldn't do any harm. Since they seem to be entirely redundant, I'll remove them from all three patches before committing. > LGTM otherwise, but please leave 24 hours for others to comment. > > Thanks, > Richard > > > switch (fcode) > > { > > case AARCH64_TME_BUILTIN_TSTART:
diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc index d0fb8bc1d1fedb382cba1a1f09a9c3ce6757ee22..f7d31d8c4308b4a883f8ce7df5c3ee319abbbb9c 100644 --- a/gcc/config/aarch64/aarch64-builtins.cc +++ b/gcc/config/aarch64/aarch64-builtins.cc @@ -1791,19 +1791,19 @@ aarch64_init_tme_builtins (void) = build_function_type_list (void_type_node, uint64_type_node, NULL); aarch64_builtin_decls[AARCH64_TME_BUILTIN_TSTART] - = aarch64_general_add_builtin ("__builtin_aarch64_tstart", + = aarch64_general_simulate_builtin ("__tstart", ftype_uint64_void, AARCH64_TME_BUILTIN_TSTART); aarch64_builtin_decls[AARCH64_TME_BUILTIN_TTEST] - = aarch64_general_add_builtin ("__builtin_aarch64_ttest", + = aarch64_general_simulate_builtin ("__ttest", ftype_uint64_void, AARCH64_TME_BUILTIN_TTEST); aarch64_builtin_decls[AARCH64_TME_BUILTIN_TCOMMIT] - = aarch64_general_add_builtin ("__builtin_aarch64_tcommit", + = aarch64_general_simulate_builtin ("__tcommit", ftype_void_void, AARCH64_TME_BUILTIN_TCOMMIT); aarch64_builtin_decls[AARCH64_TME_BUILTIN_TCANCEL] - = aarch64_general_add_builtin ("__builtin_aarch64_tcancel", + = aarch64_general_simulate_builtin ("__tcancel", ftype_void_uint64, AARCH64_TME_BUILTIN_TCANCEL); } @@ -2068,6 +2068,7 @@ handle_arm_acle_h (void) { if (TARGET_LS64) aarch64_init_ls64_builtins (); + aarch64_init_tme_builtins (); } /* Initialize fpsr fpcr getters and setters. */ @@ -2160,9 +2161,6 @@ aarch64_general_init_builtins (void) if (!TARGET_ILP32) aarch64_init_pauth_hint_builtins (); - if (TARGET_TME) - aarch64_init_tme_builtins (); - if (TARGET_MEMTAG) aarch64_init_memtag_builtins (); @@ -2289,6 +2287,7 @@ aarch64_general_check_builtin_call (location_t location, vec<location_t>, unsigned int code, tree fndecl, unsigned int nargs ATTRIBUTE_UNUSED, tree *args) { + tree decl = aarch64_builtin_decls[code]; switch (code) { case AARCH64_RSR: @@ -2301,15 +2300,28 @@ aarch64_general_check_builtin_call (location_t location, vec<location_t>, case AARCH64_WSR64: case AARCH64_WSRF: case AARCH64_WSRF64: - tree addr = STRIP_NOPS (args[0]); - if (TREE_CODE (TREE_TYPE (addr)) != POINTER_TYPE - || TREE_CODE (addr) != ADDR_EXPR - || TREE_CODE (TREE_OPERAND (addr, 0)) != STRING_CST) - { - error_at (location, "first argument to %qD must be a string literal", - fndecl); - return false; - } + { + tree addr = STRIP_NOPS (args[0]); + if (TREE_CODE (TREE_TYPE (addr)) != POINTER_TYPE + || TREE_CODE (addr) != ADDR_EXPR + || TREE_CODE (TREE_OPERAND (addr, 0)) != STRING_CST) + { + error_at (location, "first argument to %qD must be a string literal", + fndecl); + return false; + } + break; + } + + case AARCH64_TME_BUILTIN_TSTART: + case AARCH64_TME_BUILTIN_TCOMMIT: + case AARCH64_TME_BUILTIN_TTEST: + case AARCH64_TME_BUILTIN_TCANCEL: + return aarch64_check_required_extensions (location, decl, + AARCH64_FL_TME, false); + + default: + break; } /* Default behavior. */ return true; @@ -2734,6 +2746,11 @@ aarch64_expand_fcmla_builtin (tree exp, rtx target, int fcode) static rtx aarch64_expand_builtin_tme (int fcode, tree exp, rtx target) { + tree fndecl = aarch64_builtin_decls[fcode]; + if (!aarch64_check_required_extensions (EXPR_LOCATION (exp), fndecl, + AARCH64_FL_TME, false)) + return target; + switch (fcode) { case AARCH64_TME_BUILTIN_TSTART: diff --git a/gcc/config/aarch64/arm_acle.h b/gcc/config/aarch64/arm_acle.h index 2aa681090fa205449cf1ac63151565f960716189..2d84ab1bd3f3241196727d7a632a155014708081 100644 --- a/gcc/config/aarch64/arm_acle.h +++ b/gcc/config/aarch64/arm_acle.h @@ -252,10 +252,7 @@ __crc32d (uint32_t __a, uint64_t __b) #pragma GCC pop_options -#ifdef __ARM_FEATURE_TME -#pragma GCC push_options -#pragma GCC target ("+nothing+tme") - +/* Constants for TME failure reason. */ #define _TMFAILURE_REASON 0x00007fffu #define _TMFAILURE_RTRY 0x00008000u #define _TMFAILURE_CNCL 0x00010000u @@ -268,37 +265,6 @@ __crc32d (uint32_t __a, uint64_t __b) #define _TMFAILURE_INT 0x00800000u #define _TMFAILURE_TRIVIAL 0x01000000u -__extension__ extern __inline uint64_t -__attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) -__tstart (void) -{ - return __builtin_aarch64_tstart (); -} - -__extension__ extern __inline void -__attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) -__tcommit (void) -{ - __builtin_aarch64_tcommit (); -} - -__extension__ extern __inline void -__attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) -__tcancel (const uint64_t __reason) -{ - __builtin_aarch64_tcancel (__reason); -} - -__extension__ extern __inline uint64_t -__attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) -__ttest (void) -{ - return __builtin_aarch64_ttest (); -} - -#pragma GCC pop_options -#endif - #ifdef __ARM_FEATURE_LS64 typedef __arm_data512_t data512_t; #endif diff --git a/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-1.c b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-1.c new file mode 100644 index 0000000000000000000000000000000000000000..9894d3341f6bc352c22ad95d4d1e000207ca8d00 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-1.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-additional-options "-march=armv8-a" } */ + +#include <arm_acle.h> + +void foo (void) +{ + __tcommit (); /* { dg-error {ACLE function '__tcommit' requires ISA extension 'tme'} } */ +} diff --git a/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-2.c b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-2.c new file mode 100644 index 0000000000000000000000000000000000000000..4e3d69712b14a8123f45a2ead02b5048883614d9 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-2.c @@ -0,0 +1,10 @@ +/* { dg-do compile } */ +/* { dg-additional-options "-march=armv8-a" } */ + +#include <arm_acle.h> + +#pragma GCC target("arch=armv8-a+tme") +void foo (void) +{ + __tcommit (); +} diff --git a/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-3.c b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-3.c new file mode 100644 index 0000000000000000000000000000000000000000..5f480ebb8209fdaeb4baa77dbdf5467d16936644 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-3.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-additional-options "-march=armv8-a+tme -mgeneral-regs-only" } */ + +#include <arm_acle.h> + +void foo (void) +{ + __tcommit (); +} diff --git a/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-4.c b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-4.c new file mode 100644 index 0000000000000000000000000000000000000000..bf4d368370c614ffe33035d9ec4f86988f3f1c30 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/acle/tme_guard-4.c @@ -0,0 +1,10 @@ +/* { dg-do compile } */ +/* { dg-additional-options "-march=armv8-a+tme" } */ + +#include <arm_acle.h> + +#pragma GCC target("arch=armv8-a") +void foo (void) +{ + __tcommit (); /* { dg-error {ACLE function '__tcommit' requires ISA extension 'tme'} } */ +}