Skip to content
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.

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

[Driver][Sparc] Default to -mcpu=v9 for 32-bit Linux/sparc64 #109278

Merged
merged 3 commits into from
Sep 21, 2024

Conversation

rorth
Copy link
Collaborator

@rorth rorth commented Sep 19, 2024

While working on supporting PR #109101 on Linux/sparc64, I was reminded that clang -m32 still defaults to generating V8 code, although the 64-bit kernel requires a V9 CPU.

This patch corrects that on V9-only distros (Debian, Gentoo).

Tested on sparc64-unknown-linux-gnu, x86_64-pc-linux-gnu, sparcv9-sun-solaris2.11, and amd64-pc-solaris2.11.

A previous version of this patch was submitted as [Driver][Sparc] Default to -mcpu=v9 for SparcV8 on Linux, but got stalled for 2+ years.

While working on supporting PR llvm#109101 on Linux/sparc64, I was reminded
that `clang -m32` still defaults to generating V8 code, although the 64-bit
kernel requires a V9 CPU.

This patch corrects that on V9-only distros (Debian, Gentoo).

Tested on `sparc64-unknown-linux-gnu`, `x86_64-pc-linux-gnu`,
`sparcv9-sun-solaris2.11`, and `amd64-pc-solaris2.11`.

A previous version of this patch was submitted as [[Driver][Sparc] Default
to -mcpu=v9 for SparcV8 on Linux](https://reviews.llvm.org/D130688), but
got stalled for 2+ years.
@rorth rorth requested a review from MaskRay September 19, 2024 12:48
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:Sparc clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Sep 19, 2024
@rorth
Copy link
Collaborator Author

rorth commented Sep 19, 2024

@glaubitz

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 19, 2024

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-backend-sparc

@llvm/pr-subscribers-clang-driver

Author: Rainer Orth (rorth)

Changes

While working on supporting PR #109101 on Linux/sparc64, I was reminded that clang -m32 still defaults to generating V8 code, although the 64-bit kernel requires a V9 CPU.

This patch corrects that on V9-only distros (Debian, Gentoo).

Tested on sparc64-unknown-linux-gnu, x86_64-pc-linux-gnu, sparcv9-sun-solaris2.11, and amd64-pc-solaris2.11.

A previous version of this patch was submitted as [Driver][Sparc] Default to -mcpu=v9 for SparcV8 on Linux, but got stalled for 2+ years.


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

3 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/Arch/Sparc.cpp (+4-1)
  • (modified) clang/test/Preprocessor/predefined-arch-macros.c (+5-2)
  • (modified) clang/test/lit.cfg.py (+4)
diff --git a/clang/lib/Driver/ToolChains/Arch/Sparc.cpp b/clang/lib/Driver/ToolChains/Arch/Sparc.cpp
index f7f0a265fef68b..44eddc7603b4bd 100644
--- a/clang/lib/Driver/ToolChains/Arch/Sparc.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/Sparc.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "Sparc.h"
+#include "clang/Driver/Distro.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
@@ -125,7 +126,9 @@ std::string sparc::getSparcTargetCPU(const Driver &D, const ArgList &Args,
     return std::string(CPUName);
   }
 
-  if (Triple.getArch() == llvm::Triple::sparc && Triple.isOSSolaris())
+  Distro Dist(D.getVFS(), Triple);
+  if (Triple.getArch() == llvm::Triple::sparc &&
+      (Triple.isOSSolaris() || Dist.IsDebian() || Dist.IsGentoo()))
     return "v9";
   return "";
 }
diff --git a/clang/test/Preprocessor/predefined-arch-macros.c b/clang/test/Preprocessor/predefined-arch-macros.c
index a149c69ee0cdb2..2e576546b45c7a 100644
--- a/clang/test/Preprocessor/predefined-arch-macros.c
+++ b/clang/test/Preprocessor/predefined-arch-macros.c
@@ -4131,13 +4131,16 @@
 
 // RUN: %clang -E -dM %s -o - 2>&1 \
 // RUN:     -target sparc-unknown-linux \
-// RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_SPARC
+// RUN:   | FileCheck -match-full-lines %s -check-prefixes=CHECK_SPARC,CHECK_SPARC%if sparc64-distro %{_V9%} %else %{_V8%}
 // CHECK_SPARC: #define __BIG_ENDIAN__ 1
 // CHECK_SPARC: #define __sparc 1
 // CHECK_SPARC: #define __sparc__ 1
 // CHECK_SPARC-NOT: #define __sparcv9 1
 // CHECK_SPARC-NOT: #define __sparcv9__ 1
-// CHECK_SPARC: #define __sparcv8 1
+// CHECK_SPARC_V8: #define __sparcv8 1
+// CHECK_SPARC_V8-NOT: #define __sparc_v9__ 1
+// CHECK_SPARC_V9: #define __sparc_v9__ 1
+// CHECK_SPARC_V9-NOT: #define __sparcv8 1
 // CHECK_SPARC-NOT: #define __sparcv9 1
 // CHECK_SPARC-NOT: #define __sparcv9__ 1
 
diff --git a/clang/test/lit.cfg.py b/clang/test/lit.cfg.py
index 92a3361ce672e2..068975fac5a7a4 100644
--- a/clang/test/lit.cfg.py
+++ b/clang/test/lit.cfg.py
@@ -224,6 +224,10 @@ def have_host_clang_repl_cuda():
         "default-cxx-stdlib={}".format(config.clang_default_cxx_stdlib)
     )
 
+# clang -m32 defaults to -mcpu=v9 on Linux/sparc64 distros.
+if re.search(r"debian|gentoo", platform.version(), re.I):
+    config.available_features.add("sparc64-distro")
+
 # As of 2011.08, crash-recovery tests still do not pass on FreeBSD.
 if platform.system() not in ["FreeBSD"]:
     config.available_features.add("crash-recovery")

if (Triple.getArch() == llvm::Triple::sparc && Triple.isOSSolaris())
Distro Dist(D.getVFS(), Triple);
if (Triple.getArch() == llvm::Triple::sparc &&
(Triple.isOSSolaris() || Dist.IsDebian() || Dist.IsGentoo()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Pardon me for the basic question but why only for Debian and Gentoo and not for all Linux in general?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is the very popular SPARC Leon CPU that is used by ESA among other important customers and that uses V8 as the baseline. And Linux is supported and actively maintained on Leon.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly: so far I've just included distros that I know for certain are SPARC V9 only. This can always be augmented if more come up, of course.

@MaskRay
Copy link
Member

MaskRay commented Sep 20, 2024

We try to restrict distribution differences to things like default linker options and library paths. Affecting -mcpu= seems very unintuitive. There are many Debian derivatives. It's weird that Debian uses -mcpu=v9 while others use -mcpu=v8. We should not increase IsDebian or IsGentoo use.

If -mcpu=v9 seems the right thing for the majority of configurations, we can bump -mcpu=v9 for all Linux and ask other, older systems to use https://clang.llvm.org/docs/UsersManual.html#configuration-files to specify -mcpu=v8.

@thesamesam
Copy link
Member

(I'm fine with maskray's idea as well.)

@rorth
Copy link
Collaborator Author

rorth commented Sep 20, 2024

We try to restrict distribution differences to things like default linker options and library paths. Affecting -mcpu= seems very unintuitive. There are many Debian derivatives. It's weird that Debian uses -mcpu=v9 while others use -mcpu=v8. We should not increase IsDebian or IsGentoo use.

Ah, thanks. That simplifies things a lot.

If -mcpu=v9 seems the right thing for the majority of configurations, we can bump -mcpu=v9 for all Linux and ask other, older systems to use https://clang.llvm.org/docs/UsersManual.html#configuration-files to specify -mcpu=v8.

I'd been operating under the assumption that clang should work out of the box on either kind of distro. I'll update the patch accordingly and add a release notes entry about the change.

@rorth
Copy link
Collaborator Author

rorth commented Sep 20, 2024

Just to be certain: the revised version of the patch is ok to commit?

@glaubitz
Copy link
Contributor

Just to be certain: the revised version of the patch is ok to commit?

I think it would be good to get an ACK from at least one of the SPARC Leon maintainers.

CC @andreas-gaisler

@MaskRay
Copy link
Member

MaskRay commented Sep 20, 2024

Just to be certain: the revised version of the patch is ok to commit?

I think it would be good to get an ACK from at least one of the SPARC Leon maintainers.

CC @andreas-gaisler

Thanks for informing the maintainer. Note: this is done as a courtesy. When we bump a setting -mcpu= for good reasons, we do not add exception code like !isVendorA() && !isVendorB(). clangDriver is moving out of business to hard code such vendor details and requires adoption of Clang configuration files.

@rorth
Copy link
Collaborator Author

rorth commented Sep 21, 2024

Good, that considerably simplifies non-native testing:

  • The original patch ignored the issue because I had no idea how to handle it.
  • The first version of this one used the hack of checking uname -v. While this works, it's still wrong since this checks the host distro, not the target one.
  • It occured to me that one could use the vendor component of the target triple for this purpose. Right now, only SUSE is used here, but it could be extended to Debian, Gentoo and others if need be. AFAICS, the vendor field is usually just ignored.

@rorth rorth merged commit 39e3050 into llvm:main Sep 21, 2024
9 checks passed
augusto2112 pushed a commit to augusto2112/llvm-project that referenced this pull request Sep 26, 2024
…9278)

While working on supporting PR llvm#109101 on Linux/sparc64, I was reminded
that `clang -m32` still defaults to generating V8 code, although the
64-bit kernel requires a V9 CPU.

This patch corrects that.

Tested on `sparc64-unknown-linux-gnu`, `x86_64-pc-linux-gnu`,
`sparcv9-sun-solaris2.11`, and `amd64-pc-solaris2.11`.
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
…9278)

While working on supporting PR llvm#109101 on Linux/sparc64, I was reminded
that `clang -m32` still defaults to generating V8 code, although the
64-bit kernel requires a V9 CPU.

This patch corrects that.

Tested on `sparc64-unknown-linux-gnu`, `x86_64-pc-linux-gnu`,
`sparcv9-sun-solaris2.11`, and `amd64-pc-solaris2.11`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:Sparc clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants