From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 4BDEB3858D1E for ; Fri, 10 Nov 2023 10:34:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4BDEB3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 4BDEB3858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699612474; cv=none; b=WOU8xL0CZiLPNth83AXzXAzNnqyI9vZ6xYzT4tVyvmslcYI7IgwatZkXflORIS+lyqH+lEmJT8evYJcm07uVo/p5WPgtbF4B+HxTtiPI+hcpCVIYqkXN3FK6RLk9HhS/9GVmirp6ieoTFWO1bdk84XbAz82Pjn5u3FkQfSuwMt8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699612474; c=relaxed/simple; bh=4wj6DYUyqe3I8SlynxFPxq6onkn3i3rsWRKUwTOd5uM=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=lJ2E9a2mp/r2xvIabAutQTKGF9zcSsPYtApl0QwjS8RleJuPom868mlEiwPYvttGPi4V50E+FJoXY2AfWzx92tYZ8E/q7OSyT+BNK8ysrnNkd8+gspF3UEw+75CR2uHiQ+2meQEHIT8apvRfM6ArgMyehJKpkjrSBSyCIBQFPh4= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 76F59106F; Fri, 10 Nov 2023 02:35:16 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3052B3F6C4; Fri, 10 Nov 2023 02:34:31 -0800 (PST) From: Richard Sandiford To: Andrew Carlotti Mail-Followup-To: Andrew Carlotti ,gcc-patches@gcc.gnu.org, richard.earnshaw@arm.com, kyrylo.tkachov@arm.com, richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org, richard.earnshaw@arm.com, kyrylo.tkachov@arm.com Subject: Re: [2/4] aarch64: Fix tme intrinsic availability References: <6dccdd32-6c7b-5962-24e4-8acc7910a76d@e124511.cambridge.arm.com> <74c28cf9-9a02-c17b-fc97-09ff9abe9096@e124511.cambridge.arm.com> Date: Fri, 10 Nov 2023 10:34:29 +0000 In-Reply-To: <74c28cf9-9a02-c17b-fc97-09ff9abe9096@e124511.cambridge.arm.com> (Andrew Carlotti's message of "Thu, 9 Nov 2023 11:26:27 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-23.0 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_LOTSOFHASH,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Andrew Carlotti 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. > 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 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 > + > +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 > + > +#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 > + > +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 > + > +#pragma GCC target("arch=armv8-a") > +void foo (void) > +{ > + __tcommit (); /* { dg-error {ACLE function '__tcommit' requires ISA extension 'tme'} } */ > +}