Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AArch64] Fix a bug where user could not disable certain architecture features #104752

Merged

Conversation

tmatheson-arm
Copy link
Contributor

@tmatheson-arm tmatheson-arm commented Aug 19, 2024

Before this patch, some features were marked as mandatory but not be enabled by default for an architecture.
This resulted in a bug where the +nofeat in -march=base_arch+nofeat would be ignored.

This could result in a binary including instructions from extensions that the user has explicitly requested be disabled. This binary will fault at runtime on hardware that does not have these extensions.

In order to fix this, an additional check was added to the tablegen emitter, which surfaced several errors in how features were specified for CPUs and architecture versions. These had to be fixed in the same patch.

(cherry picked from commit 362142c)
(original PR #104435)

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:AArch64 clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' mc Machine (object) code labels Aug 19, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 19, 2024

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Tomas Matheson (tmatheson-arm)

Changes

This adds a check that all ExtensionWithMArch which are marked as implied features for an architecture are also present in the list of default features. It doesn't make sense to have something mandatory but not on by default.

There were a number of existing cases that violated this rule, and some changes to which features are mandatory (indicated by the Implies field).

This resulted in a bug where if a feature was marked as Implies but was not added to DefaultExt, then for -march=base_arch+nofeat the Driver would consider feat to have never been added and therefore would do nothing to disable it (no -target-feature -feat would be added, but the backend would enable the feature by default because of Implies). See
clang/test/Driver/aarch64-negative-modifiers-for-default-features.c.

Note that the processor definitions do not respect the architecture DefaultExts. These apply only when specifying -march=<some architecture version>. So when a feature is moved from Implies to DefaultExts on the Architecture definition, the feature needs to be added to all processor definitions (that are based on that architecture) in order to preserve the existing behaviour. I have checked the TRMs for many cases (see specific commit messages) but in other cases I have just kept the current behaviour and not tried to fix it.


Patch is 66.28 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/104752.diff

21 Files Affected:

  • (modified) clang/test/CodeGen/aarch64-targetattr.c (+6-6)
  • (added) clang/test/Driver/aarch64-negative-modifiers-for-default-features.c (+12)
  • (modified) clang/test/Driver/arm-sb.c (+1-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a12.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a13.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a14.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a15.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a16.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a17.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-m4.c (-2)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82ae.c (-1)
  • (modified) llvm/lib/Target/AArch64/AArch64Features.td (+9-10)
  • (modified) llvm/lib/Target/AArch64/AArch64Processors.td (+37-9)
  • (modified) llvm/test/MC/AArch64/arm64-system-encoding.s (+1-1)
  • (modified) llvm/test/MC/AArch64/armv8.5a-ssbs-error.s (+1-1)
  • (modified) llvm/test/MC/AArch64/armv8.5a-ssbs.s (+1-1)
  • (modified) llvm/test/MC/Disassembler/AArch64/armv8.5a-ssbs.txt (+1-1)
  • (modified) llvm/test/MC/Disassembler/AArch64/basic-a64-instructions.txt (+1-1)
  • (modified) llvm/unittests/TargetParser/TargetParserTest.cpp (+57-40)
  • (modified) llvm/utils/TableGen/ARMTargetDefEmitter.cpp (+29-3)
diff --git a/clang/test/CodeGen/aarch64-targetattr.c b/clang/test/CodeGen/aarch64-targetattr.c
index 4f891f938b6186..d6227be2ebef83 100644
--- a/clang/test/CodeGen/aarch64-targetattr.c
+++ b/clang/test/CodeGen/aarch64-targetattr.c
@@ -195,19 +195,19 @@ void minusarch() {}
 // CHECK: attributes #[[ATTR0]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+crc,+fp-armv8,+lse,+neon,+ras,+rdm,+v8.1a,+v8.2a,+v8a" }
 // CHECK: attributes #[[ATTR1]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+crc,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rdm,+sve,+v8.1a,+v8.2a,+v8a" }
 // CHECK: attributes #[[ATTR2]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+crc,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rdm,+sve,+sve2,+v8.1a,+v8.2a,+v8a" }
-// CHECK: attributes #[[ATTR3]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+ras,+rcpc,+rdm,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" }
-// CHECK: attributes #[[ATTR4]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="cortex-a710" "target-features"="+bf16,+complxnum,+crc,+dotprod,+ete,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+mte,+neon,+pauth,+perfmon,+ras,+rcpc,+rdm,+sb,+sve,+sve2,+sve2-bitperm,+trbe,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8a,+v9a" }
+// CHECK: attributes #[[ATTR3]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+bf16,+bti,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+predres,+ras,+rcpc,+rdm,+sb,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" }
+// CHECK: attributes #[[ATTR4]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="cortex-a710" "target-features"="+bf16,+bti,+ccidx,+complxnum,+crc,+dit,+dotprod,+ete,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+mte,+neon,+pauth,+perfmon,+predres,+ras,+rcpc,+rdm,+sb,+ssbs,+sve,+sve2,+sve2-bitperm,+trbe,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8a,+v9a" }
 // CHECK: attributes #[[ATTR5]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #[[ATTR6]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+ete,+fp-armv8,+neon,+trbe,+v8a" }
 // CHECK: attributes #[[ATTR7]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "tune-cpu"="generic" }
 // CHECK: attributes #[[ATTR8]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-n1" "target-features"="+aes,+crc,+dotprod,+fp-armv8,+fullfp16,+lse,+neon,+perfmon,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs,+v8.1a,+v8.2a,+v8a" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #[[ATTR9]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+fp-armv8,+fullfp16,+sve" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #[[ATTR10]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+ccdp,+complxnum,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8a" }
-// CHECK: attributes #[[ATTR11]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+ccdp,+complxnum,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8a,-sve" }
+// CHECK: attributes #[[ATTR10]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+ccdp,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8a" }
+// CHECK: attributes #[[ATTR11]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+ccdp,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8a,-sve" }
 // CHECK: attributes #[[ATTR12]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+fp-armv8,+fullfp16,+sve" }
 // CHECK: attributes #[[ATTR13]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+fp-armv8,+fullfp16" }
-// CHECK: attributes #[[ATTR14]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #[[ATTR15]] = { noinline nounwind optnone "branch-target-enforcement" "guarded-control-stack" "no-trapping-math"="true" "sign-return-address"="non-leaf" "sign-return-address-key"="a_key" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
+// CHECK: attributes #[[ATTR14]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+bti,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+predres,+ras,+rcpc,+rdm,+sb,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
+// CHECK: attributes #[[ATTR15]] = { noinline nounwind optnone "branch-target-enforcement" "guarded-control-stack" "no-trapping-math"="true" "sign-return-address"="non-leaf" "sign-return-address-key"="a_key" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+bti,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+predres,+ras,+rcpc,+rdm,+sb,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #[[ATTR16]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
 // CHECK: attributes #[[ATTR17]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="-v9.3a" }
 //.
diff --git a/clang/test/Driver/aarch64-negative-modifiers-for-default-features.c b/clang/test/Driver/aarch64-negative-modifiers-for-default-features.c
new file mode 100644
index 00000000000000..03dd8af7588cae
--- /dev/null
+++ b/clang/test/Driver/aarch64-negative-modifiers-for-default-features.c
@@ -0,0 +1,12 @@
+// Test that default features (e.g. flagm/sb/ssbs for 8.5) can be disabled via -march.
+
+// RUN: %clang --target=aarch64 -march=armv8.5-a+noflagm+nosb+nossbs -c %s -### 2>&1 | FileCheck %s
+// CHECK: "-triple" "aarch64"
+// CHECK-SAME: "-target-feature" "+v8.5a"
+// CHECK-SAME: "-target-feature" "-flagm"
+// CHECK-SAME: "-target-feature" "-sb"
+// CHECK-SAME: "-target-feature" "-ssbs"
+
+// CHECK-NOT: "-target-feature" "+flagm"
+// CHECK-NOT: "-target-feature" "+sb"
+// CHECK-NOT: "-target-feature" "+ssbs"
diff --git a/clang/test/Driver/arm-sb.c b/clang/test/Driver/arm-sb.c
index f2704f33c27111..9c0f381171cb6d 100644
--- a/clang/test/Driver/arm-sb.c
+++ b/clang/test/Driver/arm-sb.c
@@ -11,6 +11,6 @@
 
 // RUN: %clang -### -target arm-none-none-eabi %s 2>&1 | FileCheck %s --check-prefix=ABSENT
 // RUN: %clang -### -target aarch64-none-elf %s 2>&1 | FileCheck %s --check-prefix=ABSENT
-// RUN: %clang -### -target aarch64-none-elf -march=armv8.5a+nosb %s 2>&1 | FileCheck %s --check-prefix=ABSENT
+// RUN: %clang -### -target aarch64-none-elf -march=armv8.5a+nosb %s 2>&1 | FileCheck %s --check-prefix=NOSB
 // ABSENT-NOT: "-target-feature" "+sb"
 // ABSENT-NOT: "-target-feature" "-sb"
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a12.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a12.c
index 27f066a3107086..fcc8b674df33f4 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a12.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a12.c
@@ -6,7 +6,6 @@
 // CHECK-NEXT:     Architecture Feature(s)                                Description
 // CHECK-NEXT:     FEAT_AES, FEAT_PMULL                                   Enable AES support
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_DPB                                               Enable Armv8.2-A data Cache Clean to Point of Persistence
 // CHECK-NEXT:     FEAT_FCMA                                              Enable Armv8.3-A Floating-point complex number support
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a13.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a13.c
index 197b2102599510..dae95b1297e145 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a13.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a13.c
@@ -7,7 +7,6 @@
 // CHECK-NEXT:     FEAT_AES, FEAT_PMULL                                   Enable AES support
 // CHECK-NEXT:     FEAT_AMUv1                                             Enable Armv8.4-A Activity Monitors extension
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
 // CHECK-NEXT:     FEAT_DPB                                               Enable Armv8.2-A data Cache Clean to Point of Persistence
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a14.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a14.c
index f1731ef034a0c1..8ddcddede4110d 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a14.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a14.c
@@ -7,7 +7,6 @@
 // CHECK-NEXT:     FEAT_AES, FEAT_PMULL                                   Enable AES support
 // CHECK-NEXT:     FEAT_AMUv1                                             Enable Armv8.4-A Activity Monitors extension
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a15.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a15.c
index 267287eaf7b6e9..302661a80db30b 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a15.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a15.c
@@ -10,7 +10,6 @@
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
 // CHECK-NEXT:     FEAT_BF16                                              Enable BFloat16 Extension
 // CHECK-NEXT:     FEAT_BTI                                               Enable Branch Target Identification
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a16.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a16.c
index de382a3497b81b..a3f5150f87c287 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a16.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a16.c
@@ -10,7 +10,6 @@
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
 // CHECK-NEXT:     FEAT_BF16                                              Enable BFloat16 Extension
 // CHECK-NEXT:     FEAT_BTI                                               Enable Branch Target Identification
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a17.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a17.c
index 641aa3f82387b7..dec90ffddb6ee8 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a17.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a17.c
@@ -10,7 +10,6 @@
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
 // CHECK-NEXT:     FEAT_BF16                                              Enable BFloat16 Extension
 // CHECK-NEXT:     FEAT_BTI                                               Enable Branch Target Identification
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-m4.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-m4.c
index 5096bc6940520f..0a815174033dc8 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-m4.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-m4.c
@@ -10,7 +10,6 @@
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
 // CHECK-NEXT:     FEAT_BF16                                              Enable BFloat16 Extension
 // CHECK-NEXT:     FEAT_BTI                                               Enable Branch Target Identification
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
@@ -51,7 +50,6 @@
 // CHECK-NEXT:     FEAT_SME_F64F64                                        Enable Scalable Matrix Extension (SME) F64F64 instructions
 // CHECK-NEXT:     FEAT_SME_I16I64                                        Enable Scalable Matrix Extension (SME) I16I64 instructions
 // CHECK-NEXT:     FEAT_SPECRES                                           Enable Armv8.5-A execution and data prediction invalidation instructions
-// CHECK-NEXT:     FEAT_SSBS, FEAT_SSBS2                                  Enable Speculative Store Bypass Safe bit
 // CHECK-NEXT:     FEAT_TLBIOS, FEAT_TLBIRANGE                            Enable Armv8.4-A TLB Range and Maintenance instructions
 // CHECK-NEXT:     FEAT_TRF                                               Enable Armv8.4-A Trace extension
 // CHECK-NEXT:     FEAT_UAO                                               Enable Armv8.2-A UAO PState
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82.c b/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82.c
index 2b85201c2c6fe6..9875c6922d379a 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82.c
@@ -5,7 +5,6 @@
 // CHECK-EMPTY:
 // CHECK-NEXT:     Architecture Feature(s)                                Description
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82ae.c b/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82ae.c
index 417687b4af287d..2db44d7827aadb 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82ae.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82ae.c
@@ -5,7 +5,6 @@
 // CHECK-EMPTY:
 // CHECK-NEXT:     Architecture Feature(s)                                Description
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/llvm/lib/Target/AArch64/AArch64Features.td b/llvm/lib/Target/AArch64/AArch64Features.td
index 69af6f7efa7837..a4f8f8c2d9629f 100644
--- a/llvm/lib/Target/AArch64/AArch64Features.td
+++ b/llvm/lib/Target/AArch64/AArch64Features.td
@@ -787,27 +787,26 @@ def HasV8_2aOps : Architecture64<8, 2, "a", "v8.2a",
   [HasV8_1aOps, FeaturePsUAO, FeaturePAN_RWV, FeatureRAS, FeatureCCPP],
   !listconcat(HasV8_1aOps.DefaultExts, [FeatureRAS])>;
 def H...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Aug 19, 2024

@llvm/pr-subscribers-mc

Author: Tomas Matheson (tmatheson-arm)

Changes

This adds a check that all ExtensionWithMArch which are marked as implied features for an architecture are also present in the list of default features. It doesn't make sense to have something mandatory but not on by default.

There were a number of existing cases that violated this rule, and some changes to which features are mandatory (indicated by the Implies field).

This resulted in a bug where if a feature was marked as Implies but was not added to DefaultExt, then for -march=base_arch+nofeat the Driver would consider feat to have never been added and therefore would do nothing to disable it (no -target-feature -feat would be added, but the backend would enable the feature by default because of Implies). See
clang/test/Driver/aarch64-negative-modifiers-for-default-features.c.

Note that the processor definitions do not respect the architecture DefaultExts. These apply only when specifying -march=&lt;some architecture version&gt;. So when a feature is moved from Implies to DefaultExts on the Architecture definition, the feature needs to be added to all processor definitions (that are based on that architecture) in order to preserve the existing behaviour. I have checked the TRMs for many cases (see specific commit messages) but in other cases I have just kept the current behaviour and not tried to fix it.


Patch is 66.28 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/104752.diff

21 Files Affected:

  • (modified) clang/test/CodeGen/aarch64-targetattr.c (+6-6)
  • (added) clang/test/Driver/aarch64-negative-modifiers-for-default-features.c (+12)
  • (modified) clang/test/Driver/arm-sb.c (+1-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a12.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a13.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a14.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a15.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a16.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-a17.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-apple-m4.c (-2)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82.c (-1)
  • (modified) clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82ae.c (-1)
  • (modified) llvm/lib/Target/AArch64/AArch64Features.td (+9-10)
  • (modified) llvm/lib/Target/AArch64/AArch64Processors.td (+37-9)
  • (modified) llvm/test/MC/AArch64/arm64-system-encoding.s (+1-1)
  • (modified) llvm/test/MC/AArch64/armv8.5a-ssbs-error.s (+1-1)
  • (modified) llvm/test/MC/AArch64/armv8.5a-ssbs.s (+1-1)
  • (modified) llvm/test/MC/Disassembler/AArch64/armv8.5a-ssbs.txt (+1-1)
  • (modified) llvm/test/MC/Disassembler/AArch64/basic-a64-instructions.txt (+1-1)
  • (modified) llvm/unittests/TargetParser/TargetParserTest.cpp (+57-40)
  • (modified) llvm/utils/TableGen/ARMTargetDefEmitter.cpp (+29-3)
diff --git a/clang/test/CodeGen/aarch64-targetattr.c b/clang/test/CodeGen/aarch64-targetattr.c
index 4f891f938b6186..d6227be2ebef83 100644
--- a/clang/test/CodeGen/aarch64-targetattr.c
+++ b/clang/test/CodeGen/aarch64-targetattr.c
@@ -195,19 +195,19 @@ void minusarch() {}
 // CHECK: attributes #[[ATTR0]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+crc,+fp-armv8,+lse,+neon,+ras,+rdm,+v8.1a,+v8.2a,+v8a" }
 // CHECK: attributes #[[ATTR1]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+crc,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rdm,+sve,+v8.1a,+v8.2a,+v8a" }
 // CHECK: attributes #[[ATTR2]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+crc,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rdm,+sve,+sve2,+v8.1a,+v8.2a,+v8a" }
-// CHECK: attributes #[[ATTR3]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+ras,+rcpc,+rdm,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" }
-// CHECK: attributes #[[ATTR4]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="cortex-a710" "target-features"="+bf16,+complxnum,+crc,+dotprod,+ete,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+mte,+neon,+pauth,+perfmon,+ras,+rcpc,+rdm,+sb,+sve,+sve2,+sve2-bitperm,+trbe,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8a,+v9a" }
+// CHECK: attributes #[[ATTR3]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+bf16,+bti,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+predres,+ras,+rcpc,+rdm,+sb,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" }
+// CHECK: attributes #[[ATTR4]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="cortex-a710" "target-features"="+bf16,+bti,+ccidx,+complxnum,+crc,+dit,+dotprod,+ete,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+mte,+neon,+pauth,+perfmon,+predres,+ras,+rcpc,+rdm,+sb,+ssbs,+sve,+sve2,+sve2-bitperm,+trbe,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8a,+v9a" }
 // CHECK: attributes #[[ATTR5]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #[[ATTR6]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+ete,+fp-armv8,+neon,+trbe,+v8a" }
 // CHECK: attributes #[[ATTR7]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "tune-cpu"="generic" }
 // CHECK: attributes #[[ATTR8]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-n1" "target-features"="+aes,+crc,+dotprod,+fp-armv8,+fullfp16,+lse,+neon,+perfmon,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs,+v8.1a,+v8.2a,+v8a" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #[[ATTR9]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+fp-armv8,+fullfp16,+sve" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #[[ATTR10]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+ccdp,+complxnum,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8a" }
-// CHECK: attributes #[[ATTR11]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+ccdp,+complxnum,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8a,-sve" }
+// CHECK: attributes #[[ATTR10]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+ccdp,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8a" }
+// CHECK: attributes #[[ATTR11]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+ccdp,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8a,-sve" }
 // CHECK: attributes #[[ATTR12]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+fp-armv8,+fullfp16,+sve" }
 // CHECK: attributes #[[ATTR13]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+fp-armv8,+fullfp16" }
-// CHECK: attributes #[[ATTR14]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #[[ATTR15]] = { noinline nounwind optnone "branch-target-enforcement" "guarded-control-stack" "no-trapping-math"="true" "sign-return-address"="non-leaf" "sign-return-address-key"="a_key" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+complxnum,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
+// CHECK: attributes #[[ATTR14]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+bti,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+predres,+ras,+rcpc,+rdm,+sb,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
+// CHECK: attributes #[[ATTR15]] = { noinline nounwind optnone "branch-target-enforcement" "guarded-control-stack" "no-trapping-math"="true" "sign-return-address"="non-leaf" "sign-return-address-key"="a_key" "stack-protector-buffer-size"="8" "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+bti,+ccidx,+complxnum,+crc,+dit,+dotprod,+flagm,+fp-armv8,+fullfp16,+i8mm,+jsconv,+lse,+neon,+pauth,+perfmon,+predres,+ras,+rcpc,+rdm,+sb,+sha2,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #[[ATTR16]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
 // CHECK: attributes #[[ATTR17]] = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="-v9.3a" }
 //.
diff --git a/clang/test/Driver/aarch64-negative-modifiers-for-default-features.c b/clang/test/Driver/aarch64-negative-modifiers-for-default-features.c
new file mode 100644
index 00000000000000..03dd8af7588cae
--- /dev/null
+++ b/clang/test/Driver/aarch64-negative-modifiers-for-default-features.c
@@ -0,0 +1,12 @@
+// Test that default features (e.g. flagm/sb/ssbs for 8.5) can be disabled via -march.
+
+// RUN: %clang --target=aarch64 -march=armv8.5-a+noflagm+nosb+nossbs -c %s -### 2>&1 | FileCheck %s
+// CHECK: "-triple" "aarch64"
+// CHECK-SAME: "-target-feature" "+v8.5a"
+// CHECK-SAME: "-target-feature" "-flagm"
+// CHECK-SAME: "-target-feature" "-sb"
+// CHECK-SAME: "-target-feature" "-ssbs"
+
+// CHECK-NOT: "-target-feature" "+flagm"
+// CHECK-NOT: "-target-feature" "+sb"
+// CHECK-NOT: "-target-feature" "+ssbs"
diff --git a/clang/test/Driver/arm-sb.c b/clang/test/Driver/arm-sb.c
index f2704f33c27111..9c0f381171cb6d 100644
--- a/clang/test/Driver/arm-sb.c
+++ b/clang/test/Driver/arm-sb.c
@@ -11,6 +11,6 @@
 
 // RUN: %clang -### -target arm-none-none-eabi %s 2>&1 | FileCheck %s --check-prefix=ABSENT
 // RUN: %clang -### -target aarch64-none-elf %s 2>&1 | FileCheck %s --check-prefix=ABSENT
-// RUN: %clang -### -target aarch64-none-elf -march=armv8.5a+nosb %s 2>&1 | FileCheck %s --check-prefix=ABSENT
+// RUN: %clang -### -target aarch64-none-elf -march=armv8.5a+nosb %s 2>&1 | FileCheck %s --check-prefix=NOSB
 // ABSENT-NOT: "-target-feature" "+sb"
 // ABSENT-NOT: "-target-feature" "-sb"
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a12.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a12.c
index 27f066a3107086..fcc8b674df33f4 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a12.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a12.c
@@ -6,7 +6,6 @@
 // CHECK-NEXT:     Architecture Feature(s)                                Description
 // CHECK-NEXT:     FEAT_AES, FEAT_PMULL                                   Enable AES support
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_DPB                                               Enable Armv8.2-A data Cache Clean to Point of Persistence
 // CHECK-NEXT:     FEAT_FCMA                                              Enable Armv8.3-A Floating-point complex number support
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a13.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a13.c
index 197b2102599510..dae95b1297e145 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a13.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a13.c
@@ -7,7 +7,6 @@
 // CHECK-NEXT:     FEAT_AES, FEAT_PMULL                                   Enable AES support
 // CHECK-NEXT:     FEAT_AMUv1                                             Enable Armv8.4-A Activity Monitors extension
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
 // CHECK-NEXT:     FEAT_DPB                                               Enable Armv8.2-A data Cache Clean to Point of Persistence
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a14.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a14.c
index f1731ef034a0c1..8ddcddede4110d 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a14.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a14.c
@@ -7,7 +7,6 @@
 // CHECK-NEXT:     FEAT_AES, FEAT_PMULL                                   Enable AES support
 // CHECK-NEXT:     FEAT_AMUv1                                             Enable Armv8.4-A Activity Monitors extension
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a15.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a15.c
index 267287eaf7b6e9..302661a80db30b 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a15.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a15.c
@@ -10,7 +10,6 @@
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
 // CHECK-NEXT:     FEAT_BF16                                              Enable BFloat16 Extension
 // CHECK-NEXT:     FEAT_BTI                                               Enable Branch Target Identification
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a16.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a16.c
index de382a3497b81b..a3f5150f87c287 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a16.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a16.c
@@ -10,7 +10,6 @@
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
 // CHECK-NEXT:     FEAT_BF16                                              Enable BFloat16 Extension
 // CHECK-NEXT:     FEAT_BTI                                               Enable Branch Target Identification
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a17.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a17.c
index 641aa3f82387b7..dec90ffddb6ee8 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-a17.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-a17.c
@@ -10,7 +10,6 @@
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
 // CHECK-NEXT:     FEAT_BF16                                              Enable BFloat16 Extension
 // CHECK-NEXT:     FEAT_BTI                                               Enable Branch Target Identification
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-apple-m4.c b/clang/test/Driver/print-enabled-extensions/aarch64-apple-m4.c
index 5096bc6940520f..0a815174033dc8 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-apple-m4.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-apple-m4.c
@@ -10,7 +10,6 @@
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
 // CHECK-NEXT:     FEAT_BF16                                              Enable BFloat16 Extension
 // CHECK-NEXT:     FEAT_BTI                                               Enable Branch Target Identification
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
@@ -51,7 +50,6 @@
 // CHECK-NEXT:     FEAT_SME_F64F64                                        Enable Scalable Matrix Extension (SME) F64F64 instructions
 // CHECK-NEXT:     FEAT_SME_I16I64                                        Enable Scalable Matrix Extension (SME) I16I64 instructions
 // CHECK-NEXT:     FEAT_SPECRES                                           Enable Armv8.5-A execution and data prediction invalidation instructions
-// CHECK-NEXT:     FEAT_SSBS, FEAT_SSBS2                                  Enable Speculative Store Bypass Safe bit
 // CHECK-NEXT:     FEAT_TLBIOS, FEAT_TLBIRANGE                            Enable Armv8.4-A TLB Range and Maintenance instructions
 // CHECK-NEXT:     FEAT_TRF                                               Enable Armv8.4-A Trace extension
 // CHECK-NEXT:     FEAT_UAO                                               Enable Armv8.2-A UAO PState
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82.c b/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82.c
index 2b85201c2c6fe6..9875c6922d379a 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82.c
@@ -5,7 +5,6 @@
 // CHECK-EMPTY:
 // CHECK-NEXT:     Architecture Feature(s)                                Description
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82ae.c b/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82ae.c
index 417687b4af287d..2db44d7827aadb 100644
--- a/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82ae.c
+++ b/clang/test/Driver/print-enabled-extensions/aarch64-cortex-r82ae.c
@@ -5,7 +5,6 @@
 // CHECK-EMPTY:
 // CHECK-NEXT:     Architecture Feature(s)                                Description
 // CHECK-NEXT:     FEAT_AdvSIMD                                           Enable Advanced SIMD instructions
-// CHECK-NEXT:     FEAT_CCIDX                                             Enable Armv8.3-A Extend of the CCSIDR number of sets
 // CHECK-NEXT:     FEAT_CRC32                                             Enable Armv8.0-A CRC-32 checksum instructions
 // CHECK-NEXT:     FEAT_CSV2_2                                            Enable architectural speculation restriction
 // CHECK-NEXT:     FEAT_DIT                                               Enable Armv8.4-A Data Independent Timing instructions
diff --git a/llvm/lib/Target/AArch64/AArch64Features.td b/llvm/lib/Target/AArch64/AArch64Features.td
index 69af6f7efa7837..a4f8f8c2d9629f 100644
--- a/llvm/lib/Target/AArch64/AArch64Features.td
+++ b/llvm/lib/Target/AArch64/AArch64Features.td
@@ -787,27 +787,26 @@ def HasV8_2aOps : Architecture64<8, 2, "a", "v8.2a",
   [HasV8_1aOps, FeaturePsUAO, FeaturePAN_RWV, FeatureRAS, FeatureCCPP],
   !listconcat(HasV8_1aOps.DefaultExts, [FeatureRAS])>;
 def H...
[truncated]

@tmatheson-arm tmatheson-arm added this to the LLVM 19.X Release milestone Aug 19, 2024
@tmatheson-arm tmatheson-arm requested a review from tru August 19, 2024 09:58
@tmatheson-arm tmatheson-arm changed the title [AArch64] Add a check for invalid default features (#104435) [AArch64] Add a check for invalid default features Aug 19, 2024
Copy link
Contributor

@jthackray jthackray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This fix should definitely be in llvm19.

@DavidSpickett
Copy link
Collaborator

This needs a summary on the impact of not including the change, for folks who are not familiar with Arm's extension details.

This resulted in a bug where if a feature was marked as Implies but was not added to DefaultExt, then for -march=base_arch+nofeat the Driver would consider feat to have never been added and therefore would do nothing to disable it (no -target-feature -feat would be added, but the backend would enable the feature by default because of Implies). See
clang/test/Driver/aarch64-negative-modifiers-for-default-features.c.

So I think the impact is:
This could result in a binary including instructions from extensions that the user has explicitly requested be disabled. This binary will fault at runtime on hardware that does not have these extensions.

@DavidSpickett
Copy link
Collaborator

This adds a check that all ExtensionWithMArch which are marked as implied features for an architecture are also present in the list of default features.

And do I understand correctly that though this PR is titled "Add a check", it also fixes instances that the check discovered? So the backport is primarily to include those fixes rather than the check itself.

@tmatheson-arm
Copy link
Contributor Author

Yes both of those are correct.

@tmatheson-arm tmatheson-arm changed the title [AArch64] Add a check for invalid default features [AArch64] Fix a bug where user could not disable certain architecture features Aug 19, 2024
@tmatheson-arm
Copy link
Contributor Author

@tru is there still time to merge this into 19.x, or is there anything else I should do? I'm not familiar with the new process.

This adds a check that all ExtensionWithMArch which are marked as
implied features for an architecture are also present in the list of
default features. It doesn't make sense to have something mandatory but
not on by default.

There were a number of existing cases that violated this rule, and some
changes to which features are mandatory (indicated by the Implies
field).

This resulted in a bug where if a feature was marked as `Implies` but
was not added to `DefaultExt`, then for `-march=base_arch+nofeat` the
Driver would consider `feat` to have never been added and therefore
would do nothing to disable it (no `-target-feature -feat` would be
added, but the backend would enable the feature by default because of
`Implies`). See
clang/test/Driver/aarch64-negative-modifiers-for-default-features.c.

Note that the processor definitions do not respect the architecture
DefaultExts. These apply only when specifying `-march=<some architecture
version>`. So when a feature is moved from `Implies` to `DefaultExts` on
the Architecture definition, the feature needs to be added to all
processor definitions (that are based on that architecture) in order to
preserve the existing behaviour. I have checked the TRMs for many cases
(see specific commit messages) but in other cases I have just kept the
current behaviour and not tried to fix it.
@tru tru force-pushed the default_features_fix_cherry_pick branch from 0bcbfb6 to 263965e Compare August 20, 2024 07:19
@tru tru merged commit 263965e into llvm:release/19.x Aug 20, 2024
13 of 15 checks passed
Copy link

@tmatheson-arm (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

@tmatheson-arm tmatheson-arm deleted the default_features_fix_cherry_pick branch January 21, 2025 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category mc Machine (object) code
Projects
Development

Successfully merging this pull request may close these issues.

5 participants