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

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