[z/OS] Set the default arch for z/OS to be arch10 #89854
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-systemz @llvm/pr-subscribers-clang-driver Author: Sean Perry (perry-ca) ChangesThe default arch level on z/OS is arch10. Update the code so z/OS has arch10 without changing the default for zLinux. Full diff: https://github.com/llvm/llvm-project/pull/89854.diff 8 Files Affected:
diff --git a/clang/CMakeLists.txt b/clang/CMakeLists.txt
index f092766fa19f07..f06fbec1d02f91 100644
--- a/clang/CMakeLists.txt
+++ b/clang/CMakeLists.txt
@@ -270,6 +270,8 @@ set(CLANG_DEFAULT_OPENMP_RUNTIME "libomp" CACHE STRING
set(CLANG_SYSTEMZ_DEFAULT_ARCH "z10" CACHE STRING "SystemZ Default Arch")
+set(CLANG_SYSTEMZ_ZOS_DEFAULT_ARCH "zEC12" CACHE STRING "SystemZ z/OS Default Arch")
+
set(CLANG_VENDOR ${PACKAGE_VENDOR} CACHE STRING
"Vendor-specific text for showing with version information.")
diff --git a/clang/include/clang/Config/config.h.cmake b/clang/include/clang/Config/config.h.cmake
index 27ed69e21562bf..004c5c95c00cff 100644
--- a/clang/include/clang/Config/config.h.cmake
+++ b/clang/include/clang/Config/config.h.cmake
@@ -32,6 +32,9 @@
/* Default architecture for SystemZ. */
#define CLANG_SYSTEMZ_DEFAULT_ARCH "${CLANG_SYSTEMZ_DEFAULT_ARCH}"
+/* Default architecture for SystemZ for z/OS. */
+#define CLANG_SYSTEMZ_ZOS_DEFAULT_ARCH "${CLANG_SYSTEMZ_ZOS_DEFAULT_ARCH}"
+
/* Multilib basename for libdir. */
#define CLANG_INSTALL_LIBDIR_BASENAME "${CLANG_INSTALL_LIBDIR_BASENAME}"
diff --git a/clang/lib/Basic/Targets/SystemZ.h b/clang/lib/Basic/Targets/SystemZ.h
index 8e302acd51b8ad..45fb7e447b6429 100644
--- a/clang/lib/Basic/Targets/SystemZ.h
+++ b/clang/lib/Basic/Targets/SystemZ.h
@@ -24,7 +24,6 @@ namespace targets {
class LLVM_LIBRARY_VISIBILITY SystemZTargetInfo : public TargetInfo {
static const char *const GCCRegNames[];
- std::string CPU;
int ISARevision;
bool HasTransactionalExecution;
bool HasVector;
@@ -33,7 +32,8 @@ class LLVM_LIBRARY_VISIBILITY SystemZTargetInfo : public TargetInfo {
public:
SystemZTargetInfo(const llvm::Triple &Triple, const TargetOptions &)
- : TargetInfo(Triple), CPU("z10"), ISARevision(8),
+ : TargetInfo(Triple),
+ ISARevision(getISARevision(Triple.isOSzOS() ? "zEC12" : "z10")),
HasTransactionalExecution(false), HasVector(false), SoftFloat(false),
UnalignedSymbols(false) {
IntMaxType = SignedLong;
@@ -140,8 +140,7 @@ class LLVM_LIBRARY_VISIBILITY SystemZTargetInfo : public TargetInfo {
}
bool setCPU(const std::string &Name) override {
- CPU = Name;
- ISARevision = getISARevision(CPU);
+ ISARevision = getISARevision(Name);
return ISARevision != -1;
}
diff --git a/clang/lib/Driver/ToolChains/Arch/SystemZ.cpp b/clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
index 2213f431eb8114..7baeffc00053a7 100644
--- a/clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/SystemZ.cpp
@@ -34,7 +34,8 @@ systemz::FloatABI systemz::getSystemZFloatABI(const Driver &D,
return ABI;
}
-std::string systemz::getSystemZTargetCPU(const ArgList &Args) {
+std::string systemz::getSystemZTargetCPU(const Driver &, const ArgList &Args,
+ const llvm::Triple &T) {
if (const Arg *A = Args.getLastArg(clang::driver::options::OPT_march_EQ)) {
llvm::StringRef CPUName = A->getValue();
@@ -48,6 +49,8 @@ std::string systemz::getSystemZTargetCPU(const ArgList &Args) {
return std::string(CPUName);
}
+ if (T.isOSzOS())
+ return CLANG_SYSTEMZ_ZOS_DEFAULT_ARCH;
return CLANG_SYSTEMZ_DEFAULT_ARCH;
}
diff --git a/clang/lib/Driver/ToolChains/Arch/SystemZ.h b/clang/lib/Driver/ToolChains/Arch/SystemZ.h
index 1e42b68a8f3c20..438dab8765bc34 100644
--- a/clang/lib/Driver/ToolChains/Arch/SystemZ.h
+++ b/clang/lib/Driver/ToolChains/Arch/SystemZ.h
@@ -27,7 +27,8 @@ enum class FloatABI {
FloatABI getSystemZFloatABI(const Driver &D, const llvm::opt::ArgList &Args);
-std::string getSystemZTargetCPU(const llvm::opt::ArgList &Args);
+std::string getSystemZTargetCPU(const Driver &D, const llvm::opt::ArgList &Args,
+ const llvm::Triple &T);
void getSystemZTargetFeatures(const Driver &D, const llvm::opt::ArgList &Args,
std::vector<llvm::StringRef> &Features);
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index b65b96db16bd79..73f5abfaafcba0 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -604,7 +604,7 @@ std::string tools::getCPUName(const Driver &D, const ArgList &Args,
return getLanaiTargetCPU(Args);
case llvm::Triple::systemz:
- return systemz::getSystemZTargetCPU(Args);
+ return systemz::getSystemZTargetCPU(D, Args, T);
case llvm::Triple::r600:
case llvm::Triple::amdgcn:
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp b/clang/lib/Driver/ToolChains/Gnu.cpp
index f55b8bf48c13f7..149f4060f4fc76 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -936,7 +936,8 @@ void tools::gnutools::Assembler::ConstructJob(Compilation &C,
case llvm::Triple::systemz: {
// Always pass an -march option, since our default of z10 is later
// than the GNU assembler's default.
- std::string CPUName = systemz::getSystemZTargetCPU(Args);
+ std::string CPUName =
+ systemz::getSystemZTargetCPU(D, Args, getToolChain().getTriple());
CmdArgs.push_back(Args.MakeArgString("-march=" + CPUName));
break;
}
diff --git a/clang/test/Preprocessor/predefined-arch-macros.c b/clang/test/Preprocessor/predefined-arch-macros.c
index ca51f2fc22c517..55a6cf63a94e11 100644
--- a/clang/test/Preprocessor/predefined-arch-macros.c
+++ b/clang/test/Preprocessor/predefined-arch-macros.c
@@ -4153,6 +4153,20 @@
// Begin SystemZ/GCC/Linux tests ----------------
+// RUN: %clang -E -dM %s -o - 2>&1 \
+// RUN: -target s390x-ibm-zos \
+// RUN: | FileCheck -match-full-lines %s -check-prefix=CHECK_SYSTEMZ_ZOS
+// CHECK_SYSTEMZ_ZOS: #define __ARCH__ 10
+// CHECK_SYSTEMZ_ZOS: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
+// CHECK_SYSTEMZ_ZOS: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
+// CHECK_SYSTEMZ_ZOS: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
+// CHECK_SYSTEMZ_ZOS: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8 1
+// CHECK_SYSTEMZ_ZOS: #define __HTM__ 1
+// CHECK_SYSTEMZ_ZOS: #define __LONG_DOUBLE_128__ 1
+// CHECK_SYSTEMZ_ZOS: #define __s390__ 1
+// CHECK_SYSTEMZ_ZOS: #define __s390x__ 1
+// CHECK_SYSTEMZ_ZOS: #define __zarch__ 1
+
// RUN: %clang -march=arch8 -E -dM %s -o - 2>&1 \
// RUN: -target s390x-unknown-linux \
// RUN: | FileCheck -match-full-lines %s -check-prefix=CHECK_SYSTEMZ_ARCH8
@@ -4164,6 +4178,7 @@
// CHECK_SYSTEMZ_ARCH8: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
// CHECK_SYSTEMZ_ARCH8: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
// CHECK_SYSTEMZ_ARCH8: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8 1
+// CHECK_SYSTEMZ_ARCH8-NOT: #define __HTM__ 1
// CHECK_SYSTEMZ_ARCH8: #define __LONG_DOUBLE_128__ 1
// CHECK_SYSTEMZ_ARCH8: #define __s390__ 1
// CHECK_SYSTEMZ_ARCH8: #define __s390x__ 1
@@ -4180,6 +4195,7 @@
// CHECK_SYSTEMZ_ARCH9: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
// CHECK_SYSTEMZ_ARCH9: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
// CHECK_SYSTEMZ_ARCH9: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8 1
+// CHECK_SYSTEMZ_ARCH9-NOT: #define __HTM__ 1
// CHECK_SYSTEMZ_ARCH9: #define __LONG_DOUBLE_128__ 1
// CHECK_SYSTEMZ_ARCH9: #define __s390__ 1
// CHECK_SYSTEMZ_ARCH9: #define __s390x__ 1
|
|
We are trying to remove such cmake variables in favor of configuration file. |
I'm not sure which configuration file(s) you are referring to. I assume you mean the cache files in clang/cmake/caches. If so, I don't think we want to put it there because I might need to explain some more. If the user compiles with |
perry-ca
changed the title
|
ping @MaskRay. Thanks |
|
Clang configuration files https://clang.llvm.org/docs/UsersManual.html#configuration-files cover your use case and the feature is exactly designed to avoid such cmake default configs. https://blogs.gentoo.org/mgorny/2022/10/07/clang-in-gentoo-now-sets-default-runtimes-via-config-file/ You can add |
|
@MaskRay Got it. The problem with that solution is that if you use --target you won't get the correct arch. This would be a problem for any cross compilation. For example, say you cross compile from zLinux (which wouldn't have the config file), the arch would be arch8. And if you cross compiled from z/OS (with the config file) to zLinux then the default arch would be arch10. Both of these would be incorrect. We also use the config files for installation specific information. It is common for users to have their own config files. If we require that the option be in the config file then we would be creating a very error prone situation. We need to be able to change the arch default based on the target triple. |
|
ping @MaskRay ^^^ |
|
I removed the ability to make z/OS default arch configurable at build time. We don't really need it and this makes the code simpler. |
Sorry for the late reply. Such driver defaults via cmake variable would make testing brittle. Users expect that
Neither is immediately noticeable. In the past clang/test/Driver has had many such tests that require fixup. We have tried removing some unnecessary |
|
@MaskRay no worries. This brittle test issue was a reason I trying to keep the default for zLinux unchanged by this. I have posted a new commit that eliminates the cmake variable for z/OS. |
clang/lib/Basic/Targets.cpp
Outdated
| @@ -11,6 +11,8 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| #include "clang/Config/config.h" | |||
There was a problem hiding this comment.
This is needed for the change in SystemZ.h to replace the hard coded "z10" string with the CLANG_SYSTEMZ_DEFAULT_ARCH cmake variable. This applies to the other comment too.
There was a problem hiding this comment.
If SystemZ.h uses CLANG_SYSTEMZ_DEFAULT_ARCH, that file should have #include "clang/Config/config.h" as IWYU
There was a problem hiding this comment.
That was my preference too except clang/Config/config.h has a clause that it can only be included once and will generate an error if it is included a second time.
clang/lib/Basic/Targets/SystemZ.cpp
Outdated
| @@ -10,6 +10,8 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| #include "clang/Config/config.h" | |||
I just realized that https://reviews.llvm.org/D75914 added I think we want to avoid such CMake options/clang As an alternative, if the clang executable is at Once we drop |
I don't understand your proposal. In Ubuntu we need to set built-in default arch value, to a higher one than upstream has it at, for all builds without any toolchain configs. Such that any builds with any toolchain configs by default target a higher arch setting. arch10 is way too old and slow. and z13 might not be enough either for latest devel distributions, i.e. next development ubuntu release may be bumping further above z13. Does your proposal address above needs? |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I removed the additional use of CLANG_SYSTEMZ_DEFAULT_ARCH and left that for a separate discussion. The only code clean up left here is the elimination of the |
Yes. See this part:
We should then remove CLANG_SYSTEMZ_DEFAULT_ARCH (https://reviews.llvm.org/D75914) |
| @@ -4141,6 +4141,20 @@ | |||
|
|
|||
| // Begin SystemZ/GCC/Linux tests ---------------- | |||
|
|
|||
| // RUN: %clang -E -dM %s -o - 2>&1 \ | |||
| // RUN: -target s390x-ibm-zos \ | |||
There was a problem hiding this comment.
--target= for new driver tests.
However, this does not test clangDriver and therefore %clang_cc1 is preferred.
(You can ignore previous lines that obey the convention. I can fix them later.)
|
Requiring a config file when previously was not needed is a regression in behaviour for distributions. I guess Ubuntu would have to continue to patch this back in. Built-in defaults exist for a reason and yes at times need to be higher. This change risks performance regressions for RHEL SUSE and Ubuntu. Please call out this change in release notes. |
The default arch level on z/OS is arch10. Update the code so z/OS has arch10 without changing the default for zLinux.