[AArch64TargetParser]Fix reconstructFromParsedFeatures ignoring negative features #142236

MatzeB · 2025-05-30T23:21:08Z

The targetFeatureToExtension function used by
reconstructFromParsedFeatures only found positive +FEATURE strings,
but not negative -FEATURE strings. Extend the function to handle both
to fix reconstructFromParsedFeatures.

…ive features The `targetFeatureToExtension` function used by reconstructFromParsedFeatures only found positive `+FEATURE` strings, but not negative `-FEATURE` strings. Extend the function to handle both to fix `reconstructFromParsedFeatures`.

MatzeB · 2025-06-02T21:44:38Z

CI failures are unrelated to the change.

labrinea · 2025-06-03T10:06:54Z

I need to remind myself what is the expected behavior. Lemme take a look.

llvm/unittests/TargetParser/TargetParserTest.cpp

tmatheson-arm

Since reconstructFromParsedFeatures is intended to reconstruct the extensions bitset from the -cc1 command line, I'd like to see an integration test in addition to the TargetParserTests unittest. For example using -cc1 with --print-enabled-extensions if that is possible.

llvmbot · 2025-06-03T17:08:06Z

@llvm/pr-subscribers-clang

Author: Matthias Braun (MatzeB)

Changes

The targetFeatureToExtension function used by
reconstructFromParsedFeatures only found positive +FEATURE strings,
but not negative -FEATURE strings. Extend the function to handle both
to fix reconstructFromParsedFeatures.

Full diff: https://github.com/llvm/llvm-project/pull/142236.diff

3 Files Affected:

(added) clang/test/CodeGen/aarch64-always-inline-feature-bug.c (+8)
(modified) llvm/lib/TargetParser/AArch64TargetParser.cpp (+3-2)
(modified) llvm/unittests/TargetParser/TargetParserTest.cpp (+16)

diff --git a/clang/test/CodeGen/aarch64-always-inline-feature-bug.c b/clang/test/CodeGen/aarch64-always-inline-feature-bug.c
new file mode 100644
index 0000000000000..27c3983c66d2b
--- /dev/null
+++ b/clang/test/CodeGen/aarch64-always-inline-feature-bug.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple aarch64-- -target-feature +neon -target-feature +sve\
+// RUN:   -target-feature -sve -emit-llvm %s -o - | FileCheck %s
+
+// Reproducer for bug where clang would reject always_inline for unrelated
+// target features if they were disable with `-feature` on the command line.
+// CHECK: @bar
+__attribute__((always_inline)) __attribute__((target("neon"))) void foo() {}
+void bar() { foo(); }
diff --git a/llvm/lib/TargetParser/AArch64TargetParser.cpp b/llvm/lib/TargetParser/AArch64TargetParser.cpp
index e13c6e6d28c2b..4a2523440f0f0 100644
--- a/llvm/lib/TargetParser/AArch64TargetParser.cpp
+++ b/llvm/lib/TargetParser/AArch64TargetParser.cpp
@@ -60,7 +60,7 @@ uint64_t AArch64::getFMVPriority(ArrayRef<StringRef> Features) {
   ExtensionSet FeatureBits;
   for (const StringRef Feature : Features) {
     std::optional<FMVInfo> FMV = parseFMVExtension(Feature);
-    if (!FMV) {
+    if (!FMV && Feature.starts_with('+')) {
       if (std::optional<ExtensionInfo> Info = targetFeatureToExtension(Feature))
         FMV = lookupFMVByID(Info->ID);
     }
@@ -181,7 +181,8 @@ std::optional<AArch64::FMVInfo> AArch64::parseFMVExtension(StringRef FMVExt) {
 std::optional<AArch64::ExtensionInfo>
 AArch64::targetFeatureToExtension(StringRef TargetFeature) {
   for (const auto &E : Extensions)
-    if (TargetFeature == E.PosTargetFeature)
+    if (TargetFeature == E.PosTargetFeature ||
+        TargetFeature == E.NegTargetFeature)
       return E;
   return {};
 }
diff --git a/llvm/unittests/TargetParser/TargetParserTest.cpp b/llvm/unittests/TargetParser/TargetParserTest.cpp
index f4c93334ac682..468ef57cb5b9b 100644
--- a/llvm/unittests/TargetParser/TargetParserTest.cpp
+++ b/llvm/unittests/TargetParser/TargetParserTest.cpp
@@ -1831,6 +1831,22 @@ TEST_P(AArch64ExtensionDependenciesBaseCPUTestFixture,
   }
 }
 
+TEST(TargetParserTest, testAArch64ReconstructFromParsedFeatures) {
+  AArch64::ExtensionSet Extensions;
+  std::vector<std::string> FeatureOptions = {
+      "-sve2", "-Baz", "+sve", "+FooBar", "+sve2", "+neon", "-sve",
+  };
+  std::vector<std::string> NonExtensions;
+  Extensions.reconstructFromParsedFeatures(FeatureOptions, NonExtensions);
+
+  std::vector<std::string> NonExtensionsExpected = {"-Baz", "+FooBar"};
+  ASSERT_THAT(NonExtensions, testing::ContainerEq(NonExtensionsExpected));
+  std::vector<StringRef> Features;
+  Extensions.toLLVMFeatureList(Features);
+  std::vector<StringRef> FeaturesExpected = {"+sve2", "+neon", "-sve"};
+  ASSERT_THAT(FeaturesExpected, testing::ContainerEq(FeaturesExpected));
+}
+
 AArch64ExtensionDependenciesBaseArchTestParams
     AArch64ExtensionDependenciesArchData[] = {
         // Base architecture features

MatzeB · 2025-06-03T17:11:05Z

Cannot test with --print-enabled-extensions as clang has its own logic to parse target features.

But I added a test based on the always_inline compatibility check logic that uses reconstructFromParsedFeatures that is close to the original issue we ran into.

…rser

labrinea · 2025-06-05T18:34:24Z

llvm/lib/TargetParser/AArch64TargetParser.cpp

@@ -60,7 +60,7 @@ uint64_t AArch64::getFMVPriority(ArrayRef<StringRef> Features) {
  ExtensionSet FeatureBits;
  for (const StringRef Feature : Features) {
    std::optional<FMVInfo> FMV = parseFMVExtension(Feature);
-    if (!FMV) {
+    if (!FMV && Feature.starts_with('+')) {


Is it possible to have empty string? If not then Feature[0] == '+' should also work.

I don't think it's explicitly stated that empty strings are forbidden, so wouldn't like to just make assumptions also as I don't expect this function to performance critical...

labrinea · 2025-06-05T19:41:38Z

llvm/unittests/TargetParser/TargetParserTest.cpp

+  ASSERT_THAT(NonExtensions, testing::ContainerEq(NonExtensionsExpected));
+  std::vector<StringRef> Features;
+  Extensions.toLLVMFeatureList(Features);
+  std::vector<StringRef> FeaturesExpected = {"+neon", "-sve", "+sve2"};


I would expect the following:

Touched: sve2, sve, neon Enabled: neon

If an extension is Touched but not Enabled it should be printed with a '-'. That said I don't understand why we expect +sve2 and not -sve2. I would expect -sve to transitively disable sve2 since it is dependant.

This particular interface does not seem to resolve dependencies.

The point I was trying to make with the test was that having -sve2 followed by a +sve2 you end up with +sve2. While having a +sve followed by a -sve will end up with -sve etc.

I thought we were trying to fix reconstructFromParsedFeatures so that it honors negative features. If you look at the code it does Enabled.reset which should remove a feature and its dependencies.

If I am not mistaken at the point of calling reconstructFromParsedFeatures the command line arguments should already be processed having no contradictions like -featureA, +featureA (or reversed).

If you look at the code it does Enabled.reset which should remove a feature and its dependencies.

Actually I was mistaken, Enabled.reset should not transitively remove dependencies. Callingdisable() would do that, but as I said this is probably redundant since the command line arguments have been processed at this stage.

This code inside reconstructFromParsedFeatures seems dead

if (IsNegated) Enabled.reset(AE->ID);

tmatheson-arm

Thanks for adding the test. Unfortunately because it looks invalid to me, I still don't understand what the user facing problem is. Can you give a clang driver command line + C file that exhibits it?

tmatheson-arm · 2025-06-06T13:08:45Z

clang/test/CodeGen/aarch64-always-inline-feature-bug.c

+// RUN: %clang_cc1 -triple aarch64-- -target-feature +neon -target-feature +sve\
+// RUN:   -target-feature -sve -emit-llvm %s -o - | FileCheck %s


The driver should never emit a -cc1 command line with -target-feature +sve -target-feature -sve, so I don't think this is a valid way to test whatever the problem is.

tmatheson-arm · 2025-06-06T13:11:33Z

clang/test/CodeGen/aarch64-always-inline-feature-bug.c

+// Reproducer for bug where clang would reject always_inline for unrelated
+// target features if they were disable with `-feature` on the command line.
+// CHECK: @bar
+__attribute__((always_inline)) __attribute__((target("neon"))) void foo() {}


neon is not actually a valid argument here, it should be simd.

https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html#index-march

tmatheson-arm · 2025-06-06T13:16:59Z

clang/test/CodeGen/aarch64-always-inline-feature-bug.c

+
+// Reproducer for bug where clang would reject always_inline for unrelated
+// target features if they were disable with `-feature` on the command line.
+// CHECK: @bar


It would be good to check the LLVM IR attributes for both functions, which is what reconstructFromParsedFeatures is used to construct.

tmatheson-arm · 2025-06-06T13:19:55Z

llvm/unittests/TargetParser/TargetParserTest.cpp

+TEST(TargetParserTest, testAArch64ReconstructFromParsedFeatures) {
+  AArch64::ExtensionSet Extensions;
+  std::vector<std::string> FeatureOptions = {
+      "-sve2", "-Baz", "+sve", "+FooBar", "+sve2", "+neon", "-sve",


I think this has the same issue as described for the C test: the function is not designed to be able to handle both +feat and -feat in the same list, because the driver would never emit that.

MatzeB requested review from pratlucas, ostannard and tmatheson-arm May 30, 2025 23:21

MatzeB marked this pull request as ready for review May 30, 2025 23:28

labrinea self-requested a review June 3, 2025 10:07

labrinea reviewed Jun 3, 2025

View reviewed changes

llvm/unittests/TargetParser/TargetParserTest.cpp Outdated Show resolved Hide resolved

tmatheson-arm reviewed Jun 3, 2025

View reviewed changes

llvm/unittests/TargetParser/TargetParserTest.cpp Show resolved Hide resolved

tmatheson-arm reviewed Jun 3, 2025

View reviewed changes

Add a test on the clang side as well

34d6e7f

llvmbot added the clang Clang issues not falling into any other category label Jun 3, 2025

fix unit test

1a9f8a3

Merge remote-tracking branch 'github/main' into fix_aarch64_target_pa…

650461d

…rser

MatzeB requested review from labrinea and tmatheson-arm June 4, 2025 15:17

labrinea reviewed Jun 5, 2025

View reviewed changes

tmatheson-arm reviewed Jun 6, 2025

View reviewed changes

		// RUN: %clang_cc1 -triple aarch64-- -target-feature +neon -target-feature +sve\
		// RUN: -target-feature -sve -emit-llvm %s -o - \| FileCheck %s

[AArch64TargetParser]Fix reconstructFromParsedFeatures ignoring negative features #142236

Are you sure you want to change the base?

[AArch64TargetParser]Fix reconstructFromParsedFeatures ignoring negative features #142236

Conversation

MatzeB commented May 30, 2025

Uh oh!

MatzeB commented Jun 2, 2025

Uh oh!

labrinea commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

tmatheson-arm left a comment

Choose a reason for hiding this comment

Uh oh!

llvmbot commented Jun 3, 2025

Uh oh!

MatzeB commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatzeB Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmatheson-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MatzeB commented Jun 3, 2025 •

edited

Loading

MatzeB Jun 5, 2025 •

edited

Loading