Skip to content

[AArch64TargetParser]Fix reconstructFromParsedFeatures ignoring negative features #142236

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 16, 2025
Merged

[AArch64TargetParser]Fix reconstructFromParsedFeatures ignoring negative features #142236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92da4de
[AArch64TargetParser]Fix reconstructFromParsedFeatures ignoring negat…
MatzeB May 30, 2025
34d6e7f
Add a test on the clang side as well
MatzeB Jun 3, 2025
1a9f8a3
fix unit test
MatzeB Jun 3, 2025
650461d
Merge remote-tracking branch 'github/main' into fix_aarch64_target_pa…
MatzeB Jun 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions clang/test/CodeGen/aarch64-always-inline-feature-bug.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Comment on lines +1 to +2
Copy link
Contributor

@tmatheson-arm tmatheson-arm Jun 6, 2025

Choose a reason for hiding this comment

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

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.


// 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
Copy link
Contributor

@tmatheson-arm tmatheson-arm Jun 6, 2025

Choose a reason for hiding this comment

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

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

__attribute__((always_inline)) __attribute__((target("neon"))) void foo() {}
Copy link
Contributor

@tmatheson-arm tmatheson-arm Jun 6, 2025

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

@MatzeB MatzeB Jun 6, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@tmatheson-arm tmatheson-arm Jun 9, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@tmatheson-arm tmatheson-arm Jun 9, 2025

Choose a reason for hiding this comment

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

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

void bar() { foo(); }
5 changes: 3 additions & 2 deletions llvm/lib/TargetParser/AArch64TargetParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 {};
}
Expand Down
16 changes: 16 additions & 0 deletions llvm/unittests/TargetParser/TargetParserTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

@tmatheson-arm tmatheson-arm Jun 6, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@MatzeB MatzeB Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@tmatheson-arm tmatheson-arm Jun 9, 2025

Choose a reason for hiding this comment

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

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.

};
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 = {"+neon", "-sve", "+sve2"};
ASSERT_THAT(Features, testing::ContainerEq(FeaturesExpected));
}

AArch64ExtensionDependenciesBaseArchTestParams
AArch64ExtensionDependenciesArchData[] = {
// Base architecture features
Expand Down