[AArch64] Handle negative architecture features

[dmgreen]

Summary

Currently negative architecture features passes to clang like -Xclang -target-feature -Xclang -v9.3a will end up _enabling_ dependant target features (like FEAT_MOPS). This patch fixes that by ensuring we don't enable dependant target features when !Enabled.

Fixes #60375

[pratlucas]

clang/lib/Basic/Targets/AArch64.cpp

688–692

I believe these changes require a rebase. llvm::AArch64::INVALID was removed by D142539.

[tmatheson]

It looks like this has these odd behaviours:

• -target-feature +v9.3a -target-feature -v9.2a will remove v9.2a but add all the dependent features of both v9.3a and v9.2a. It also doesn't remove v9.3a which implies v9.2a.
• -target-feature -v9.2a -target-feature +v9.3a will add the v9.2a back, and also add all the dependent features of both v9.3a and v9.2a.
• -target-feature +v9.3a -target-feature -v9.3a will remove v9.3a but add all the dependent features of v9.3a.

This seems like a bit of a minefield. Is there even a use case for negative architectural features?

clang/lib/Basic/Targets/AArch64.cpp

688–693

[dmgreen]

In D142963#4093094, @tmatheson wrote:

It looks like this has these odd behaviours:

• -target-feature +v9.3a -target-feature -v9.2a will remove v9.2a but add all the dependent features of both v9.3a and v9.2a. It also doesn't remove v9.3a which implies v9.2a.
• -target-feature -v9.2a -target-feature +v9.3a will add the v9.2a back, and also add all the dependent features of both v9.3a and v9.2a.
• -target-feature +v9.3a -target-feature -v9.3a will remove v9.3a but add all the dependent features of v9.3a.

This seems like a bit of a minefield. Is there even a use case for negative architectural features?

Yes - We don't have _any_ dependencies for negative target features at the moment. That is a larger job that will take some time to do properly though.

Between architecture revisions I would say that we can trust whatever is passing the parameters to get it right (not provide contradictory results). Otherwise it is a problem elsewhere. We just need to make sure we don't treat them invalidly. In this case from the linked bug, the features are coming from zig. Clang should always only use positive features. Which is why this bug exists - we didn't expect them to be passed as negative features. We can at least not get the dependent features wrong though.

[dmgreen]

Rebase and address comments.

[andrewrk]

In D142963#4093094, @tmatheson wrote:

It looks like this has these odd behaviours:

• -target-feature +v9.3a -target-feature -v9.2a will remove v9.2a but add all the dependent features of both v9.3a and v9.2a. It also doesn't remove v9.3a which implies v9.2a.
• -target-feature -v9.2a -target-feature +v9.3a will add the v9.2a back, and also add all the dependent features of both v9.3a and v9.2a.
• -target-feature +v9.3a -target-feature -v9.3a will remove v9.3a but add all the dependent features of v9.3a.

This seems like a bit of a minefield. Is there even a use case for negative architectural features?

Speaking as the one who filed the motivating bug report, all of the above behaviors are fine. The motivating use case is explicitly specifying a full set of enabled/disabled features, leaving nothing changed by LLVM's own dependency resolution. In this use case, LLVM would never see any of these three scenarios as input.

[tmatheson]

In D142963#4094545, @andrewrk wrote:

Speaking as the one who filed the motivating bug report, all of the above behaviors are fine. The motivating use case is explicitly specifying a full set of enabled/disabled features, leaving nothing changed by LLVM's own dependency resolution. In this use case, LLVM would never see any of these three scenarios as input.

This is not something that I would recommend that you do, at least until we have a coherent model for what enabling/disabling features via -target-features means with respect to backend features. Currently:

• Architecture features are treated specially in some cases,
• it's not clear what negative features mean with respect to dependent features,
• the order in which you specify the -target-features changes the results,
• etc.

We don't test any combinations that we don't expect out of the clang driver (these are the first clang tests ever added for negative architecture features). You might have more reliable results if you stick to only using negative features where they are needed to disable features that were implicitly enabled.

clang/lib/Basic/Targets/AArch64.cpp

void AArch64TargetInfo::setFeatureEnabled(llvm::StringMap<bool> &Features,
                                           StringRef Name, bool Enabled) const {
   Features[Name] = Enabled;
   // If the feature is an architecture feature (like v8.2a), add all previous
   // architecture versions and any dependant target features.
   const std::optional<llvm::AArch64::ArchInfo> ArchInfo =
       llvm::AArch64::ArchInfo::findBySubArch(Name);
 
   if (!ArchInfo)
-    return; // Not an architecure, nothing more to do.
+    return; // Not an architecture, nothing more to do.
+
+  // Disabling an architecture feature does not affect dependent features
+  if (!Enabled)
+    return;
 
   for (const auto *OtherArch : llvm::AArch64::ArchInfos)
     if (ArchInfo->implies(*OtherArch))
-      Features[OtherArch->getSubArch()] = Enabled;
+      Features[OtherArch->getSubArch()] = true;
 
   // Set any features implied by the architecture
   std::vector<StringRef> CPUFeats;
   if (llvm::AArch64::getExtensionFeatures(ArchInfo->DefaultExts, CPUFeats)) {
     for (auto F : CPUFeats) {
       assert(F[0] == '+' && "Expected + in target feature!");
       Features[F.drop_front(1)] = true;
     }

clang/test/CodeGen/aarch64-targetattr.c

 void plusnosimd() {}
 // CHECK-LABEL: @noneon() #17
 __attribute__((target("no-neon")))
 void noneon() {}
 // CHECK-LABEL: @nosimd() #17
 __attribute__((target("no-simd")))
 void nosimd() {}
 
+// This isn't part of the standard interface, but test that -arch features should not apply anything else.
+// CHECK-LABEL: @minusarch() #18
+__attribute__((target("no-v9.3a")))
+void minusarch() {}
+
 // CHECK: attributes #0 = { {{.*}} "target-features"="+crc,+fp-armv8,+lse,+neon,+ras,+rdm,+v8.1a,+v8.2a,+v8a" }
 // CHECK: attributes #1 = { {{.*}} "target-features"="+crc,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rdm,+sve,+v8.1a,+v8.2a,+v8a" }
 // CHECK: attributes #2 = { {{.*}} "target-features"="+crc,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rdm,+sve,+sve2,+v8.1a,+v8.2a,+v8a" }
 // CHECK: attributes #3 = { {{.*}} "target-features"="+bf16,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+lse,+neon,+ras,+rcpc,+rdm,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" }
 // CHECK: attributes #4 = { {{.*}} "target-cpu"="cortex-a710" "target-features"="+bf16,+crc,+dotprod,+flagm,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+lse,+mte,+neon,+pauth,+ras,+rcpc,+rdm,+sb,+sve,+sve2,+sve2-bitperm" }
 // CHECK: attributes #5 = { {{.*}} "tune-cpu"="cortex-a710" }
 // CHECK: attributes #6 = { {{.*}} "target-cpu"="generic" }
 // CHECK: attributes #7 = { {{.*}} "tune-cpu"="generic" }
 // CHECK: attributes #8 = { {{.*}} "target-cpu"="neoverse-n1" "target-features"="+aes,+crc,+dotprod,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rcpc,+rdm,+sha2,+spe,+ssbs" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #9 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #10 = { {{.*}} "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+lse,+neon,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+sve,+sve2" }
 // CHECK: attributes #11 = { {{.*}} "target-cpu"="neoverse-v1" "target-features"="+aes,+bf16,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+lse,+neon,+rand,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,-sve" }
 // CHECK: attributes #12 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve" }
 // CHECK: attributes #13 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve,-sve2" }
 // CHECK: attributes #14 = { {{.*}} "target-features"="+fullfp16" }
 // CHECK: attributes #15 = { {{.*}} "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+lse,+neon,+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 #16 = { {{.*}} "branch-target-enforcement"="true" {{.*}} "target-features"="+aes,+bf16,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+lse,+neon,+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 #17 = { {{.*}} "target-features"="-neon" }
+// CHECK: attributes #18 = { {{.*}} "target-features"="-v9.3a" }

clang/test/Preprocessor/aarch64-target-features.c

 // RUN: %clang -target aarch64-arm-none-eabi -march=armv8.8-a+nomops+mops -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv8.8-a+mops        -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv8.8-a+mops+nomops -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOMOPS %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv9.2-a             -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOMOPS %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv9.2-a+mops        -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv9.3-a             -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv9.3-a+nomops      -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOMOPS %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv9.3-a+mops        -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-MOPS   %s
+// Check that -target-feature -v9.3a doesn't enable dependant features
+// RUN: %clang -target aarch64-arm-none-eabi -Xclang -target-feature -Xclang -v9.3a  -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOMOPS   %s
 // CHECK-MOPS: __ARM_FEATURE_MOPS 1
 // CHECK-NOMOPS-NOT: __ARM_FEATURE_MOPS 1
 
 // ================== Check Armv8.9-A/Armv9.4-A 128-bit System Registers (FEAT_SYSREG128)
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv8.9-a      -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOSYS128 %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv9.4-a      -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-NOSYS128 %s
 // RUN: %clang -target aarch64-arm-none-eabi -march=armv9.4-a+d128 -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-SYS128   %s
 // CHECK-SYS128: __ARM_FEATURE_SYSREG128 1