On Fri, Nov 10, 2023 at 10:34:29AM +0000, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlo...@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): Remove feature check.
> > (aarch64_check_general_builtin_call): New.
> > (aarch64_expand_builtin_tme): Check feature availability.
> > * config/aarch64/aarch64-c.cc (aarch64_check_builtin_call): Add
> > check for non-SVE builtins.
> > * config/aarch64/aarch64-protos.h (aarch64_check_general_builtin_call):
> > New prototype.
> > * config/aarch64/arm_acle.h (__tstart, __tcommit, __tcancel)
> > (__ttest): Remove.
> > (_TMFAILURE_*): Define unconditionally.
>
> My main concern with this is that it makes the functions available
> even without including the header file. That's fine from a namespace
> pollution PoV, since the functions are in the implementation namespace.
> But it might reduce code portability if GCC allows these ACLE functions
> to be used without including the header file, while other compilers
> require the header file.
>
> For LS64 we instead used a pragma to trigger the definition of the
> functions (requiring aarch64_general_simulate_builtin rather than
> aarch64_general_add_builtin). I think it'd be better to do the same here.
Good point - this is also the same as some simd intrinsic stuff I changed last
year. I'll fix this in an updated patch, which will then also need a slightly
different version for backporting.
> > 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
> > 11a9ba2256f105d8cb9cdc4d6decb5b2be3d69af..ac0259a892e16adb5b241032ac3df1e7ab5370ef
> > 100644
> > --- a/gcc/config/aarch64/aarch64-builtins.cc
> > +++ b/gcc/config/aarch64/aarch64-builtins.cc
> > @@ -1765,19 +1765,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_add_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_add_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_add_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_add_builtin ("__tcancel",
> > ftype_void_uint64,
> > AARCH64_TME_BUILTIN_TCANCEL);
> > }
> > @@ -2034,8 +2034,7 @@ aarch64_general_init_builtins (void)
> > if (!TARGET_ILP32)
> > aarch64_init_pauth_hint_builtins ();
> >
> > - if (TARGET_TME)
> > - aarch64_init_tme_builtins ();
> > + aarch64_init_tme_builtins ();
> >
> > if (TARGET_MEMTAG)
> > aarch64_init_memtag_builtins ();
> > @@ -2137,6 +2136,24 @@ aarch64_check_required_extensions (location_t
> > location, tree fndecl,
> > gcc_unreachable ();
> > }
> >
> > +bool aarch64_check_general_builtin_call (location_t location,
> > + unsigned int fcode)
>
> Formatting trivia: should be a line break after the "bool". Would be
> worth having a comment like:
>
> /* Implement TARGET_CHECK_BUILTIN_CALL for the AARCH64_BUILTIN_GENERAL
> group. */
>
> "aarch64_general_check_builtin_call" would avoid splitting the name
> of the target hook.
>
> Thanks,
> Richard
>
> > +{
> > + tree fndecl = aarch64_builtin_decls[fcode];
> > + switch (fcode)
> > + {
> > + 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, fndecl,
> > + AARCH64_FL_TME, false);
> > +
> > + default:
> > + break;
> > + }
> > + return true;
> > +}
> >
> > typedef enum
> > {
> > @@ -2559,6 +2576,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/aarch64-c.cc
> > b/gcc/config/aarch64/aarch64-c.cc
> > index
> > ab8844f6049dc95b97648b651bfcd3a4ccd3ca0b..6b6bd77e9e66cd2d9a211387e07d3e20d935fb1a
> > 100644
> > --- a/gcc/config/aarch64/aarch64-c.cc
> > +++ b/gcc/config/aarch64/aarch64-c.cc
> > @@ -339,7 +339,7 @@ aarch64_check_builtin_call (location_t loc,
> > vec<location_t> arg_loc,
> > switch (code & AARCH64_BUILTIN_CLASS)
> > {
> > case AARCH64_BUILTIN_GENERAL:
> > - return true;
> > + return aarch64_check_general_builtin_call (loc, subcode);
> >
> > case AARCH64_BUILTIN_SVE:
> > return aarch64_sve::check_builtin_call (loc, arg_loc, subcode,
> > diff --git a/gcc/config/aarch64/aarch64-protos.h
> > b/gcc/config/aarch64/aarch64-protos.h
> > index
> > 30726140a6945dcb86b787f8f47952810d99379f..94022f77d7e7bab2533d78965bec241b4070c729
> > 100644
> > --- a/gcc/config/aarch64/aarch64-protos.h
> > +++ b/gcc/config/aarch64/aarch64-protos.h
> > @@ -991,6 +991,8 @@ void handle_arm_neon_h (void);
> > bool aarch64_check_required_extensions (location_t, tree,
> > aarch64_feature_flags, bool = true);
> >
> > +bool aarch64_check_general_builtin_call (location_t, unsigned int);
> > +
> > namespace aarch64_sve {
> > void init_builtins ();
> > void handle_arm_sve_h ();
> > diff --git a/gcc/config/aarch64/arm_acle.h b/gcc/config/aarch64/arm_acle.h
> > index
> > 7599a32301dadf80760d3cb40a8685d2e6a476fb..f4e35d1e12ac9bbcc4f1b75d8e5baad62f8634a0
> > 100644
> > --- a/gcc/config/aarch64/arm_acle.h
> > +++ b/gcc/config/aarch64/arm_acle.h
> > @@ -222,10 +222,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
> > @@ -238,37 +235,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'} } */
> > +}
