[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

llvm/lib/TargetParser/AArch64TargetParser.cpp

llvm/unittests/TargetParser/TargetParserTest.cpp

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

The __attribute__((target("neon"))) comes originally from the arm_neon.h header shipping with clang where this is used all over.

That's true, but the header shouldn't be doing that. neon is the LLVM subtarget feature name which shouldn't be exposed to the user, simd is the user-facing name that is used on the command line with -march. target attribute parameters should match the command line, but historically it accepts any LLVM subtarget feature, which we are trying to clean up. Because neon is used here rather than simd, I believe it is not recognised as an extension and just gets passed through to LLVM as-is, and therefore takes a different codepath.

That said, since the header does use it we should check that it behaves sensibly.

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.

This line made me believe it was meant to handle negative features: https://github.com/llvm/llvm-project/blob/main/llvm/lib/TargetParser/AArch64TargetParser.cpp#L368

I think the original intention was for it to be able to handle either positive (added to the ExtensionSet without dependencies) or negative (marked as "touched" on the ExtensionSet) for a given feature, but not see both of them in the same list. As @labrinea pointed out though the code is not perfect, the negative branch looks dead since targetFeatureToExtension doesn't handle negative features.

MatzeB · 2025-06-06T17:22:13Z

I submitted this fix after seeing this line: https://github.com/llvm/llvm-project/blob/main/llvm/lib/TargetParser/AArch64TargetParser.cpp#L368 which looks like the function intends to handle negative features.

MatzeB · 2025-06-06T17:33:16Z

The original situation looks something like:

#include <arm_neon.h>

void foo() {
   // ...
   uint8x16_t x = {};
   vreinterpretq_u8_s8(x);
}

Compiled with clang -target aarch64-redhat-linux-gnu -march=-march=armv9-a+sve2+fp16+fp16fml+crypto+bf16+sm4+i8mm+sve2-bitperm+sve2-sha3+sve2-aes+sve2-sm4 -Xclang -target-feature -Xclang -sve -c test.c (coming from a build system where -target, -march flags are set centrally but this particular target/team not being able to handle SVE appended the -Xclang flags for their target)...

This compiled fine in clang-17, but with the clang-19 the frontend started producing this confusing message (because this is a neon intrinsic, not an SVE one):

test.cpp:5:3: error: always_inline function 'vreinterpretq_u8_s8' requires target feature 'sve', but would be inlined into function 'foo' that is compiled without support for 'sve'
    5 |   vreinterpretq_u8_s8(x);

labrinea · 2025-06-09T19:50:37Z

The original situation looks something like:
#include <arm_neon.h>

void foo() {
   // ...
   uint8x16_t x = {};
   vreinterpretq_u8_s8(x);
}
Compiled with clang -target aarch64-redhat-linux-gnu -march=-march=armv9-a+sve2+fp16+fp16fml+crypto+bf16+sm4+i8mm+sve2-bitperm+sve2-sha3+sve2-aes+sve2-sm4 -Xclang -target-feature -Xclang -sve -c test.c (coming from a build system where -target, -march flags are set centrally but this particular target/team not being able to handle SVE appended the -Xclang flags for their target)...

This compiled fine in clang-17, but with the clang-19 the frontend started producing this confusing message (because this is a neon intrinsic, not an SVE one):
test.cpp:5:3: error: always_inline function 'vreinterpretq_u8_s8' requires target feature 'sve', but would be inlined into function 'foo' that is compiled without support for 'sve'
    5 |   vreinterpretq_u8_s8(x);

Well, with your patch this now compiles, but the unit test demonstrates that none of the extensions of that command line which depend on sve (like sve2, sve2-bitperm, etc) will get disabled. I doubt this was the intention. Passing internal frontend options with -Xclang is not recommended.

I tried modifyingreconstructFromParsedFeatures by replacing Enabled.reset() with disable()and Enabled.set() with enable()but that made 50 clang tests fail with errors similar to the one you reported (mismatch between target features of caller and callee).

tmatheson-arm

Sorry for the slow responses, this has revealed some issues that we are not sure how to fix... I am happy with the patch as it is, it is a strict improvement for reconstructFromParsedFeatures.

As @labrinea pointed out though, you should be aware that disabling SVE like this via -Xclang will bypass the driver, where the dependency tracking logic exists, and disable SVE only. Any subtarget features that depend on SVE (e.g. SVE2) will still be enabled, unless you explicitly disable them too. It is strongly advised to modify the -march flag instead.

Also be aware that you can't (currently) rely on --print-enabled-extensions to give you an accurate view of what is enabled/disabled if you are manually adding -target-feature flags to the -cc1 command.

See #143570 for the details.

MatzeB · 2025-06-16T19:56:56Z

Yeah I'm definitely not happy about the -Xclang -target-feature usage in our project (and I am aware of the downsides).

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

llvm/lib/TargetParser/AArch64TargetParser.cpp Show resolved Hide resolved

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

tmatheson-arm reviewed Jun 6, 2025

View reviewed changes

tmatheson-arm approved these changes Jun 11, 2025

View reviewed changes

MatzeB merged commit b0378e7 into llvm:main Jun 16, 2025
11 checks passed

		// 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

[AArch64TargetParser]Fix reconstructFromParsedFeatures ignoring negative features #142236

Uh oh!

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!

Uh oh!

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!

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 6, 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!

MatzeB commented Jun 6, 2025

Uh oh!

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

Uh oh!

Uh oh!

labrinea commented Jun 9, 2025

Uh oh!

tmatheson-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

MatzeB commented Jun 3, 2025 •

edited

Loading

MatzeB Jun 6, 2025 •

edited

Loading

MatzeB commented Jun 6, 2025 •

edited

Loading

MatzeB commented Jun 16, 2025 •

edited

Loading