Skip to content

[Clang] Remove AArch64TargetInfo::setArchFeatures #146107

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Clang] Remove AArch64TargetInfo::setArchFeatures #146107

wants to merge 1 commit into from
+5 −76

Conversation

sdesmalen-arm
Copy link
Collaborator

When compiling with -march=armv9-a+nosve we found that Clang still defines the __ARM_FEATURE_SVE2 macro, which is explicitly set in setArchFeatures when compiling for armv9-a.

After some experimenting, I found out that the list of features passed into AArch64TargetInfo::handleTargetFeatures has already been expanded and takes into account +no[feature] and has already expanded features like armv9-a.

From that I conclude that setArchFeatures is no longer required.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 27, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2025

@llvm/pr-subscribers-backend-aarch64

Author: Sander de Smalen (sdesmalen-arm)

Changes

When compiling with -march=armv9-a+nosve we found that Clang still defines the __ARM_FEATURE_SVE2 macro, which is explicitly set in setArchFeatures when compiling for armv9-a.

After some experimenting, I found out that the list of features passed into AArch64TargetInfo::handleTargetFeatures has already been expanded and takes into account +no[feature] and has already expanded features like armv9-a.

From that I conclude that setArchFeatures is no longer required.

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

2 Files Affected:

  • (modified) clang/lib/Basic/Targets/AArch64.cpp (-74)
  • (modified) clang/lib/Basic/Targets/AArch64.h (-2)
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index 124b340b62d9f..e57feafe3ae24 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -125,79 +125,6 @@ static constexpr auto BuiltinAArch64Infos =
 #include "clang/Basic/BuiltinsAArch64.def"
     });
 
-void AArch64TargetInfo::setArchFeatures() {
-  if (*ArchInfo == llvm::AArch64::ARMV8R) {
-    HasDotProd = true;
-    HasDIT = true;
-    HasFlagM = true;
-    HasRCPC = true;
-    FPU |= NeonMode;
-    HasCCPP = true;
-    HasCRC = true;
-    HasLSE = true;
-    HasRDM = true;
-  } else if (ArchInfo->Version.getMajor() == 8) {
-    if (ArchInfo->Version.getMinor() >= 7u) {
-      HasWFxT = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 6u) {
-      HasBFloat16 = true;
-      HasMatMul = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 5u) {
-      HasAlternativeNZCV = true;
-      HasFRInt3264 = true;
-      HasSSBS = true;
-      HasSB = true;
-      HasPredRes = true;
-      HasBTI = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 4u) {
-      HasDotProd = true;
-      HasDIT = true;
-      HasFlagM = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 3u) {
-      HasRCPC = true;
-      FPU |= NeonMode;
-    }
-    if (ArchInfo->Version.getMinor() >= 2u) {
-      HasCCPP = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 1u) {
-      HasCRC = true;
-      HasLSE = true;
-      HasRDM = true;
-    }
-  } else if (ArchInfo->Version.getMajor() == 9) {
-    if (ArchInfo->Version.getMinor() >= 2u) {
-      HasWFxT = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 1u) {
-      HasBFloat16 = true;
-      HasMatMul = true;
-    }
-    FPU |= SveMode;
-    HasSVE2 = true;
-    HasFullFP16 = true;
-    HasAlternativeNZCV = true;
-    HasFRInt3264 = true;
-    HasSSBS = true;
-    HasSB = true;
-    HasPredRes = true;
-    HasBTI = true;
-    HasDotProd = true;
-    HasDIT = true;
-    HasFlagM = true;
-    HasRCPC = true;
-    FPU |= NeonMode;
-    HasCCPP = true;
-    HasCRC = true;
-    HasLSE = true;
-    HasRDM = true;
-  }
-}
-
 AArch64TargetInfo::AArch64TargetInfo(const llvm::Triple &Triple,
                                      const TargetOptions &Opts)
     : TargetInfo(Triple), ABI("aapcs") {
@@ -1266,7 +1193,6 @@ bool AArch64TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
   }
 
   setDataLayout();
-  setArchFeatures();
 
   if (HasNoFP) {
     FPU &= ~FPUMode;
diff --git a/clang/lib/Basic/Targets/AArch64.h b/clang/lib/Basic/Targets/AArch64.h
index 1951e0679d2ec..56adfa97efb1a 100644
--- a/clang/lib/Basic/Targets/AArch64.h
+++ b/clang/lib/Basic/Targets/AArch64.h
@@ -158,8 +158,6 @@ class LLVM_LIBRARY_VISIBILITY AArch64TargetInfo : public TargetInfo {
     return false;
   }
 
-  void setArchFeatures();
-
   void getTargetDefinesARMV81A(const LangOptions &Opts,
                                MacroBuilder &Builder) const;
   void getTargetDefinesARMV82A(const LangOptions &Opts,

@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2025

@llvm/pr-subscribers-clang

Author: Sander de Smalen (sdesmalen-arm)

Changes

When compiling with -march=armv9-a+nosve we found that Clang still defines the __ARM_FEATURE_SVE2 macro, which is explicitly set in setArchFeatures when compiling for armv9-a.

After some experimenting, I found out that the list of features passed into AArch64TargetInfo::handleTargetFeatures has already been expanded and takes into account +no[feature] and has already expanded features like armv9-a.

From that I conclude that setArchFeatures is no longer required.

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

2 Files Affected:

  • (modified) clang/lib/Basic/Targets/AArch64.cpp (-74)
  • (modified) clang/lib/Basic/Targets/AArch64.h (-2)
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index 124b340b62d9f..e57feafe3ae24 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -125,79 +125,6 @@ static constexpr auto BuiltinAArch64Infos =
 #include "clang/Basic/BuiltinsAArch64.def"
     });
 
-void AArch64TargetInfo::setArchFeatures() {
-  if (*ArchInfo == llvm::AArch64::ARMV8R) {
-    HasDotProd = true;
-    HasDIT = true;
-    HasFlagM = true;
-    HasRCPC = true;
-    FPU |= NeonMode;
-    HasCCPP = true;
-    HasCRC = true;
-    HasLSE = true;
-    HasRDM = true;
-  } else if (ArchInfo->Version.getMajor() == 8) {
-    if (ArchInfo->Version.getMinor() >= 7u) {
-      HasWFxT = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 6u) {
-      HasBFloat16 = true;
-      HasMatMul = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 5u) {
-      HasAlternativeNZCV = true;
-      HasFRInt3264 = true;
-      HasSSBS = true;
-      HasSB = true;
-      HasPredRes = true;
-      HasBTI = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 4u) {
-      HasDotProd = true;
-      HasDIT = true;
-      HasFlagM = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 3u) {
-      HasRCPC = true;
-      FPU |= NeonMode;
-    }
-    if (ArchInfo->Version.getMinor() >= 2u) {
-      HasCCPP = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 1u) {
-      HasCRC = true;
-      HasLSE = true;
-      HasRDM = true;
-    }
-  } else if (ArchInfo->Version.getMajor() == 9) {
-    if (ArchInfo->Version.getMinor() >= 2u) {
-      HasWFxT = true;
-    }
-    if (ArchInfo->Version.getMinor() >= 1u) {
-      HasBFloat16 = true;
-      HasMatMul = true;
-    }
-    FPU |= SveMode;
-    HasSVE2 = true;
-    HasFullFP16 = true;
-    HasAlternativeNZCV = true;
-    HasFRInt3264 = true;
-    HasSSBS = true;
-    HasSB = true;
-    HasPredRes = true;
-    HasBTI = true;
-    HasDotProd = true;
-    HasDIT = true;
-    HasFlagM = true;
-    HasRCPC = true;
-    FPU |= NeonMode;
-    HasCCPP = true;
-    HasCRC = true;
-    HasLSE = true;
-    HasRDM = true;
-  }
-}
-
 AArch64TargetInfo::AArch64TargetInfo(const llvm::Triple &Triple,
                                      const TargetOptions &Opts)
     : TargetInfo(Triple), ABI("aapcs") {
@@ -1266,7 +1193,6 @@ bool AArch64TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
   }
 
   setDataLayout();
-  setArchFeatures();
 
   if (HasNoFP) {
     FPU &= ~FPUMode;
diff --git a/clang/lib/Basic/Targets/AArch64.h b/clang/lib/Basic/Targets/AArch64.h
index 1951e0679d2ec..56adfa97efb1a 100644
--- a/clang/lib/Basic/Targets/AArch64.h
+++ b/clang/lib/Basic/Targets/AArch64.h
@@ -158,8 +158,6 @@ class LLVM_LIBRARY_VISIBILITY AArch64TargetInfo : public TargetInfo {
     return false;
   }
 
-  void setArchFeatures();
-
   void getTargetDefinesARMV81A(const LangOptions &Opts,
                                MacroBuilder &Builder) const;
   void getTargetDefinesARMV82A(const LangOptions &Opts,

@jthackray
Copy link
Contributor

Thanks @sdesmalen-arm. We'd like to move all these kinds of dependencies into tablegen, and have nothing encoded in C++, and I've got a (half-started) diff to do this, but haven't found time to prioritise it yet. Your change helps, though! LGTM.

tmatheson-arm
tmatheson-arm approved these changes Jun 27, 2025
View reviewed changes
Copy link
Contributor

@tmatheson-arm tmatheson-arm left a comment

Choose a reason for hiding this comment

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

I'm surprised this has no effect, but if so LGTM. Are any of the values set here now dead?

@davemgreen
Copy link
Collaborator

The best kind of patch.

When compiling with -march=armv9-a+nosve we found that Clang still defines the __ARM_FEATURE_SVE2 macro

Could we add a test for this, in case it comes up again in the future? Negative features have not worked super well in the past and could do with extra test coverage.

@sdesmalen-arm
[Clang] Remove AArch64TargetInfo::setArchFeatures
c44ab03 
When compiling with `-march=armv9-a+nosve` we found that Clang
still defines the `__ARM_FEATURE_SVE2` macro, which is explicitly
set in `setArchFeatures` when compiling for armv9-a.

After some experimenting, I found out that the list of features
passed into `AArch64TargetInfo::handleTargetFeatures` has already
been expanded and takes into account `+no[feature]` and has
already expanded features like `armv9-a`.

From that I conclude that `setArchFeatures` is no longer required.
@sdesmalen-arm sdesmalen-arm force-pushed the clang-remove-aarch64-setarchfeatures branch from 525a4c5 to c44ab03 Compare June 29, 2025 18:57
davemgreen
davemgreen reviewed Jun 29, 2025
View reviewed changes
Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

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

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davemgreen davemgreen davemgreen left review comments

@jthackray jthackray jthackray approved these changes

@tmatheson-arm tmatheson-arm tmatheson-arm approved these changes

@labrinea labrinea Awaiting requested review from labrinea

Assignees
No one assigned
Labels
backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sdesmalen-arm @llvmbot @jthackray @davemgreen @tmatheson-arm