Re: [2/4] aarch64: Fix tme intrinsic availability

[Andrew Carlotti <andrew.carlotti@arm.com>]

On Fri, Nov 10, 2023 at 10:34:29AM +0000, 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): 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'} } */
> > +}