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 12C123858CD1 for ; Sat, 9 Dec 2023 19:09:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 12C123858CD1 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 12C123858CD1 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=1702148999; cv=none; b=nu1EAVUAn6cjspEhot0JUJPocYCw71oBUNErL5Md1/yKMgbWAhZ0BShwUgsC5/hAFRd87F55RiY98ZkNjTZs1969GSB4nzP848yY56KiYQjRGBZx+ILDwXWv2yQaNOoZeBZS8JgRMIN8KZgpnltBgbdkEKW8tv0H7KBQIpy18VA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702148999; c=relaxed/simple; bh=8s+ewqREYAMT6Bej2Psxnsa/XfDjQY4aiNyWKzOkqM4=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=pvF+fjhzQCz5WMVdW53t75DQZzvMTy9r6//mwoCRkjps86kfva+Qxj5pG3+MTsYs1teMFHvaNDOm78N5GgOi/saIE4EmhvawdPbxek5/vwpgO9/oAB7C5Pioqc8z5lsC3OPYK71LmDPUtsIUuCLDS/mIIWx/2SBslXiRhbmAz+s= 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 6F2FE1042; Sat, 9 Dec 2023 11:10:43 -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 365D13F5A1; Sat, 9 Dec 2023 11:09:57 -0800 (PST) From: Richard Sandiford To: Andrew Carlotti Mail-Followup-To: Andrew Carlotti ,gcc-patches@gcc.gnu.org, Richard Earnshaw , richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org, Richard Earnshaw Subject: Re: aarch64: Fix +nocrypto handling References: <58ac7f4d-04d3-260c-1612-1ca09c420ce5@e124511.cambridge.arm.com> Date: Sat, 09 Dec 2023 19:09:55 +0000 In-Reply-To: <58ac7f4d-04d3-260c-1612-1ca09c420ce5@e124511.cambridge.arm.com> (Andrew Carlotti's message of "Tue, 5 Dec 2023 19:33:33 +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=-21.8 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: > Additionally, replace all checks for the AARCH64_FL_CRYPTO bit with > checks for (AARCH64_FL_AES | AARCH64_FL_SHA2) instead. The value of the > AARCH64_FL_CRYPTO bit within isa_flags is now ignored, but it is > retained because removing it would make processing the data in > option-extensions.def significantly more complex. > > Ok for master? > > gcc/ChangeLog: > > * common/config/aarch64/aarch64-common.cc > (aarch64_get_extension_string_for_isa_flags): Fix generation of > the "+nocrypto" extension. > * config/aarch64/aarch64.h (AARCH64_ISA_CRYPTO): Remove. > (TARGET_CRYPTO): Remove. > * config/aarch64/aarch64-c.cc (aarch64_update_cpp_builtins): > Don't use TARGET_CRYPTO. > > gcc/testsuite/ChangeLog: > > * gcc.target/aarch64/options_set_27.c: New test. > * gcc.target/aarch64/options_set_28.c: New test. > > diff --git a/gcc/common/config/aarch64/aarch64-common.cc b/gcc/common/config/aarch64/aarch64-common.cc > index 20bc4e1291bba9b73798398fea659f1154afa205..6d12454143cd64ebaafa7f5e6c23869ee0bfa543 100644 > --- a/gcc/common/config/aarch64/aarch64-common.cc > +++ b/gcc/common/config/aarch64/aarch64-common.cc > @@ -310,6 +310,7 @@ aarch64_get_extension_string_for_isa_flags > But in order to make the output more readable, it seems better > to add the strings in definition order. */ > aarch64_feature_flags added = 0; > + auto flags_crypto = AARCH64_FL_AES | AARCH64_FL_SHA2; > for (unsigned int i = ARRAY_SIZE (all_extensions); i-- > 0; ) > { > auto &opt = all_extensions[i]; > @@ -319,7 +320,7 @@ aarch64_get_extension_string_for_isa_flags > per-feature crypto flags. */ > auto flags = opt.flag_canonical; > if (flags == AARCH64_FL_CRYPTO) > - flags = AARCH64_FL_AES | AARCH64_FL_SHA2; > + flags = flags_crypto; > > if ((flags & isa_flags & (explicit_flags | ~current_flags)) == flags) > { > @@ -337,9 +338,27 @@ aarch64_get_extension_string_for_isa_flags > /* Remove the features in current_flags & ~isa_flags. If the feature does > not have an HWCAPs then it shouldn't be taken into account for feature > detection because one way or another we can't tell if it's available > - or not. */ > + or not. > + > + As a special case, emit "+nocrypto" instead of "+noaes+nosha2", in order > + to support assemblers that predate the separate per-feature crypto flags. > + Only use "+nocrypto" when "simd" is enabled (to avoid redundant feature > + removal), and when "sm4" is not already enabled (to avoid dependending on > + whether "+nocrypto" also disables "sm4") */ > + for (auto &opt : all_extensions) > + if ((opt.flag_canonical == AARCH64_FL_CRYPTO) > + && ((flags_crypto & current_flags & ~isa_flags) == flags_crypto) > + && (current_flags & AARCH64_FL_SIMD) > + && !(current_flags & AARCH64_FL_SM4)) > + { > + current_flags &= ~opt.flags_off; > + outstr += "+no"; > + outstr += opt.name; > + } > + Is it an important part of the patch that we do this ahead of time, rather than in the main loop? Doing it in the main loop feels more natural, and should avoid the need for the SIMD test. It we do use an in-loop test, I assume the test would need to be something like: (opt.flag_canonical & flag_crypto) && (flags_crypto & current_flags & ~isa_flags) == flags_crypto && !(current_flags & AARCH64_FL_SM4) so that the new code is applied when the loop first sees a crypto flag. The set of flags to disable would be: current_flags &= ~feature_deps::get_flags_off (flag_crypto); Otherwise it looks good, thanks. As a general formatting note, GCC style is not to put individual comparisons in parentheses in && and || combos. Richard > for (auto &opt : all_extensions) > if (opt.native_detect_p > + && (opt.flag_canonical != AARCH64_FL_CRYPTO) > && (opt.flag_canonical & current_flags & ~isa_flags)) > { > current_flags &= ~opt.flags_off; > diff --git a/gcc/config/aarch64/aarch64-c.cc b/gcc/config/aarch64/aarch64-c.cc > index ab8844f6049dc95b97648b651bfcd3a4ccd3ca0b..4f9ee01d52f3ac42f95edbb030bdb2d09fc36d16 100644 > --- a/gcc/config/aarch64/aarch64-c.cc > +++ b/gcc/config/aarch64/aarch64-c.cc > @@ -140,7 +140,7 @@ aarch64_update_cpp_builtins (cpp_reader *pfile) > aarch64_def_or_undef (TARGET_ILP32, "_ILP32", pfile); > aarch64_def_or_undef (TARGET_ILP32, "__ILP32__", pfile); > > - aarch64_def_or_undef (TARGET_CRYPTO, "__ARM_FEATURE_CRYPTO", pfile); > + aarch64_def_or_undef (TARGET_AES && TARGET_SHA2, "__ARM_FEATURE_CRYPTO", pfile); > aarch64_def_or_undef (TARGET_SIMD_RDMA, "__ARM_FEATURE_QRDMX", pfile); > aarch64_def_or_undef (TARGET_SVE, "__ARM_FEATURE_SVE", pfile); > cpp_undef (pfile, "__ARM_FEATURE_SVE_BITS"); > diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h > index 1ac298926ce1606a87bcdcaf691f182ca416d600..d3613a0a42b7b6d2c4452739841b133014909a39 100644 > --- a/gcc/config/aarch64/aarch64.h > +++ b/gcc/config/aarch64/aarch64.h > @@ -177,10 +177,13 @@ enum class aarch64_feature : unsigned char { > > #endif > > -/* Macros to test ISA flags. */ > +/* Macros to test ISA flags. > + > + There is intentionally no macro for AARCH64_FL_CRYPTO, since this flag bit > + is not always set when its constituent features are present. > + Check (TARGET_AES && TARGET_SHA2) instead. */ > > #define AARCH64_ISA_CRC (aarch64_isa_flags & AARCH64_FL_CRC) > -#define AARCH64_ISA_CRYPTO (aarch64_isa_flags & AARCH64_FL_CRYPTO) > #define AARCH64_ISA_FP (aarch64_isa_flags & AARCH64_FL_FP) > #define AARCH64_ISA_SIMD (aarch64_isa_flags & AARCH64_FL_SIMD) > #define AARCH64_ISA_LSE (aarch64_isa_flags & AARCH64_FL_LSE) > @@ -223,9 +226,6 @@ enum class aarch64_feature : unsigned char { > #define AARCH64_ISA_LS64 (aarch64_isa_flags & AARCH64_FL_LS64) > #define AARCH64_ISA_CSSC (aarch64_isa_flags & AARCH64_FL_CSSC) > > -/* Crypto is an optional extension to AdvSIMD. */ > -#define TARGET_CRYPTO (AARCH64_ISA_CRYPTO) > - > /* SHA2 is an optional extension to AdvSIMD. */ > #define TARGET_SHA2 (AARCH64_ISA_SHA2) > > diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_27.c b/gcc/testsuite/gcc.target/aarch64/options_set_27.c > new file mode 100644 > index 0000000000000000000000000000000000000000..08f2b5962ad5f4204eca4d2020ace74dbfd7c7ea > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/options_set_27.c > @@ -0,0 +1,11 @@ > +/* { dg-do compile } */ > +/* { dg-additional-options "-march=armv8.2-a+aes+sha2" } */ > + > +int main () > +{ > + return 0; > +} > + > +/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crc\+crypto\n} 1 } } */ > + > +/* Checking if enabling default features drops the superfluous bits. */ > diff --git a/gcc/testsuite/gcc.target/aarch64/options_set_28.c b/gcc/testsuite/gcc.target/aarch64/options_set_28.c > new file mode 100644 > index 0000000000000000000000000000000000000000..ec7619c6c937f44bc5a3ddc29c93ecfa5dafa2f5 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/options_set_28.c > @@ -0,0 +1,11 @@ > +/* { dg-do compile } */ > +/* { dg-additional-options "-march=armv8.2-a+aes+sha3" } */ > + > +int main () > +{ > + return 0; > +} > + > +/* { dg-final { scan-assembler-times {\.arch armv8\.2\-a\+crc\+aes\+sha3\n} 1 } } */ > + > +/* Checking if enabling default features drops the superfluous bits. */