systemz: allow configuring default CLANG_SYSTEMZ_ARCH

[• xnox]

Summary

On Ubuntu, we want to raise default CLANG_SYSTEMZ_ARCH to z13, thus allow configuring this via CMake.
On Debian, we want to raise it to z196.

[uweigand]

Thanks for working on this! A few comments inline.

CMakeLists.txt

311

Should be moved above the set(CLANG_VENDOR, ... line.

Also, this new variable itself have a set line, so that we can provide a descriptive text, and also move the "z10" default to here instead of the sources.

Finally, I think the mixed-case name doesn't really match the common naming conventions here, what about CLANG_SYSTEMZ_DEFAULT_ARCH ?

[• xnox]

In D75914#1914571, @uweigand wrote:

Thanks for working on this! A few comments inline.

Updated! @uweigand Please re-review.

[uweigand]

LGTM, thanks!

[• xnox]

I cannot commit the patch myself, please "sponsor" committing this change.

[uweigand]

This doesn't apply cleanly to current mainline. Can you rebase and test again? I'll check it in then.

[• xnox]

Rebased I think. If it helps here is the github pull-request too https://github.com/llvm/llvm-project/pull/196

Git format patch https://github.com/xnox/llvm-project/commit/a6122cd8dc1b10bcba751f851cf5b1751cff165c.patch

Or $ git pull https://github.com/xnox/llvm-project D75914

[Hahnfeld]

clang/CMakeLists.txt

311 ↗

(On Diff #253559)

Passing values like this is unusual for CMake and causes all source files to be recompiled if the value is changed. Instead could we add this to include/clang/Config/config.h.cmake like other CLANG_DEFAULT_* options?

[uweigand]

Ah, good point. Dimitry, can you prepare an updated patch to implement Jonas' suggestion?

[thakis]

clang/CMakeLists.txt

311 ↗

(On Diff #253559)

+1

[thakis]

clang/CMakeLists.txt

311 ↗

(On Diff #253559)

Also, this is used in one cpp file; passing the define to every compilation unit seems a bit aggressive.

I'll try to land a patch to fix this.

[thakis]

clang/CMakeLists.txt

311 ↗

(On Diff #253559)

Done in c506adcdf2ca3ba6459e52e09c55868e3b57af46. (This only changes the implementation of the feature; the way people who build clang tell cmake what they want didn't change.)

[• xnox]

Sorry for being too slow =) I like all the fixes done to move the define into a more natural config.h location, thank you.

[uweigand]

Thanks for working on this, @thakis !

CMakeLists.txt

if (NOT DEFINED MATCHED_ARCH OR "${CMAKE_MATCH_1}" LESS 35)
   message(WARNING "Resetting default architecture for OpenMP offloading to Nvidia GPUs to sm_35")
   set(CLANG_OPENMP_NVPTX_DEFAULT_ARCH "sm_35" CACHE STRING
     "Default architecture for OpenMP offloading to Nvidia GPUs." FORCE)
 endif()
 
 set(CLANG_VENDOR ${PACKAGE_VENDOR} CACHE STRING
   "Vendor-specific text for showing with version information.")
 
+if( CLANG_SystemZTargetCPU )
+  add_definitions( -DCLANG_SystemZTargetCPU="${CLANG_SystemZTargetCPU}" )
+endif()
+
 if( CLANG_VENDOR )
   add_definitions( -DCLANG_VENDOR="${CLANG_VENDOR} " )
 endif()
 
 set(CLANG_REPOSITORY_STRING "" CACHE STRING
   "Vendor-specific text for showing the repository the source is taken from.")
 
 if(CLANG_REPOSITORY_STRING)

lib/Driver/ToolChains/Arch/SystemZ.cpp

 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
 const char *systemz::getSystemZTargetCPU(const ArgList &Args) {
   if (const Arg *A = Args.getLastArg(clang::driver::options::OPT_march_EQ))
     return A->getValue();
+#if defined(CLANG_SystemZTargetCPU)
+  return CLANG_SystemZTargetCPU;
+#else
   return "z10";
+#endif
 }
 
 void systemz::getSystemZTargetFeatures(const ArgList &Args,
                                        std::vector<llvm::StringRef> &Features) {
   // -m(no-)htm overrides use of the transactional-execution facility.
   if (Arg *A = Args.getLastArg(options::OPT_mhtm, options::OPT_mno_htm)) {
     if (A->getOption().matches(options::OPT_mhtm))
       Features.push_back("+transactional-execution");