Configure target flags without modifying global space #20

kwryankrattiger · 2021-08-20T21:05:30Z

@ax3l I think this will fix the downstream package issues, but I am still finishing testing some other workflows.

CMake/SENSEIConfig.cmake.in

kwryankrattiger · 2021-08-31T20:27:52Z

@burlen When you get a chance could you finish reviewing these changes? I would like to get them in to fix some down stream CI.

c-wetterer-nelson · 2021-09-01T03:10:28Z

+1 on that review. this is affecting WarpX.

CMake/build.cmake

CMake/SENSEIConfig.cmake.in

CMake/build.cmake

ax3l · 2021-09-28T23:16:42Z

CMake/build.cmake

 if (NOT MSVC)
-  if (NOT CMAKE_CXX_FLAGS)
-  set(tmp "-fPIC -std=c++11 -Wall -Wextra")
+  string(APPEND CMAKE_CXX_FLAGS "-Wall -Wextra")


Consider not modifying CMAKE_ variables and instead using target_compile_options et al.

CMake/build.cmake

ax3l · 2021-09-28T23:24:05Z

Thanks a lot for working on modernizing this @kwryankrattiger and @burlen ! 👍

I have a few more comments added that are likely to cause problems in larger software stacks that would integrate you.

burlen · 2021-09-29T18:25:36Z

Thanks a lot for working on modernizing this @kwryankrattiger and @burlen ! +1

I have a few more comments added that are likely to cause problems in larger software stacks that would integrate you.

Thanks for your comments!

We could potentially get rid of setting clang stdlib and FORCE'ing the cache. Setting the clang stdlib was required on older versions of mac os. However, I don't recall if it was related to Apple's clang or brew'd clang. I think we need to do some testing on Apple to verify that both Apple and brew'd clang works without the flag.

The code prior to the changes requested in this PR would only touch CMAKE_CXX_FLAGS if they are not already set. This gives one the chance to override our defaults as needed. One can override our defaults by either passing -DCMAKE_CXX_FLAGS=<your overrides> on the cmake line, or by setting the environment variable export CXXFLAGS=<your overrrides> in the shell. Do you have any comment on why these pathways to override are not sufficient?

In addition, we should only add -O3 -march=native -mtune=native when -DCMAKE_BUILD_TYPE=Release.

ax3l · 2021-10-14T20:52:46Z

To answer the question what compilers we support on WarpX: generally, the platform specific system compiler.

That means macOS:

AppleClang

Windows: (purely informational, since you don't support this platform)

MSVC and
LLVMClang with MSVC frontend (clang-cl)

Linux: the recommended compiler for every HPC system we run on

GCC (g++)
Clang (clang++)
AMD Clang for HIP (clang++ -x hip)
Intel DPC++ Clang for SYCL/DPC++ (dpcpp / icpx -fsycl)
Intel ICX Clang (icpx)
Intel ICC (icpc)
Cray Clang
IBM Clang
Fujitsu Clang (FCC -Nclang)
Nvidia nvcc + -ccbin of g++/icpx/clang++/nvc++ for Summit/Perlmutter/Other HPC systems
not yet, but planned to re-establish: mainline Clang CUDA (clang++ -x cuda)
not yet, but soon: NVHPC CUDA nvc++ -x cuda

(WarpX: Cray, IBM and Fujitsu support is the only thing I can currently not cover in CI.)

ax3l · 2021-10-14T21:13:32Z

We could potentially get rid of setting clang stdlib and FORCE'ing the cache. Setting the clang stdlib was required on older versions of mac os. However, I don't recall if it was related to Apple's clang or brew'd clang. I think we need to do some testing on Apple to verify that both Apple and brew'd clang works without the flag.

Sounds good, have added an inline comment what to check for with regards to CMake compiler identification.

The code prior to the changes requested in this PR would only touch CMAKE_CXX_FLAGS if they are not already set. This gives one the chance to override our defaults as needed. One can override our defaults by either passing -DCMAKE_CXX_FLAGS= on the cmake line, or by setting the environment variable export CXXFLAGS= in the shell. Do you have any comment on why these pathways to override are not sufficient?

Modifying CMAKE_CXX_FLAGS will work as long as a CMake project does not include you as super-build via add_subdirectory, where then it will cause surprises for the main project.
Note that some of the flags you set are expressible more general via CMake target options now, which widens your compiler & platform support automatically.

In addition, we should only add -O3 -march=native -mtune=native when -DCMAKE_BUILD_TYPE=Release.

No, microarchitecture tuning should only be set if opted in, in my opinion. See comment here for the workflows and deployments that will break if added by default:
#20 (comment)

kwryankrattiger · 2021-11-04T01:55:57Z

Finally getting back around to this, sorry about the long hiatus. I am going to go through all of the comments here in more detail. Thanks @ax3l and @burlen for your comments! I think there is a lot of great feedback here!

I agree on the micro-architecture optimization being better as an opt-in. Unless there is an extremely strong argument not to, I think I am going to go the way of the user needs to provide CXXFLAGS="-march=native -mtune=native" cmake ... since this seems to be the safest bet, especially since SENSEI is going into the ECP SDK.

Adding -O3 is probably fine...although I am still curious as to why CMake chooses to use -O2 on some platforms. My guess is the compiler only actually goes up to level 2 optimizations and setting a higher level is simply truncated by the compiler.

It is a little annoying to set target properties...but I don't think that will be a major problem. The tests are already behind a cmake macro, and there aren't too many targets to wrangle.

kwryankrattiger · 2021-11-04T17:19:39Z

@burlen @ax3l If you guys don't mind taking one last look at this, I should have resolved the current issues that were brought up in discussion.

I also did some updating/testing on the CUDA stuff. The biggest thing was I lowered the standard to 11 for oscillators because the nvidia toolkit v10 on my system doesn't support 17, and 17 is not required to build the sample code.

burlen · 2021-11-08T17:38:10Z

Modifying CMAKE_CXX_FLAGS will work as long as a CMake project does not include you as super-build via add_subdirectory, where then it will cause surprises for the main project. Note that some of the flags you set are expressible more general via CMake target options now, which widens your compiler & platform support automatically.

SENSEI will only modify the cxx flags if they are not already set. So all one needs to do is set them to something and SENSEI will not modify them. Note that this can include setting to an empty string when you want nothing.

The purpose of setting the CXX flags is to give most of our users the best default options.

In any case, the compile options that we used to compile SENSEI should not impact Warp at all. Can you explain why Warp is using SENSEI's compile flags? If it's something we are inadvertently exporting, then I think the fix is to stop exporting them rather than change what works for most users.

burlen · 2021-11-08T17:45:04Z

@burlen @ax3l If you guys don't mind taking one last look at this, I should have resolved the current issues that were brought up in discussion.

I also did some updating/testing on the CUDA stuff. The biggest thing was I lowered the standard to 11 for oscillators because the nvidia toolkit v10 on my system doesn't support 17, and 17 is not required to build the sample code.

@kwryankrattiger This patch has too many different things in it, and some of which aren't quite ready. Please don't keep packing more stuff on this. Make a new PR for new changes so they don't get held up.

While the code will compile with only C++11, there is a feature that we used that's only available in CUDA 11 and on. Without this the code will compile but will not work correctly. I didn't see a way in CMake to require CUDA 11, so requiring C++17 (introduced in CUDA 11) seemed like a good alternative.

Can you (in a different PR) tell CMake that we require CUDA 11 or newer? In that case we could use C++11.

burlen · 2021-11-08T18:49:51Z

To answer the question what compilers we support on WarpX: generally, the platform specific system compiler.

That means macOS:

AppleClang

Windows: (purely informational, since you don't support this platform)

MSVC and

LLVMClang with MSVC frontend (clang-cl)

Linux: the recommended compiler for every HPC system we run on

GCC (g++)

Clang (clang++)

AMD Clang for HIP (clang++ -x hip)

Intel DPC++ Clang for SYCL/DPC++ (dpcpp / icpx -fsycl)

Intel ICX Clang (icpx)

Intel ICC (icpc)

Cray Clang

IBM Clang

Fujitsu Clang (FCC -Nclang)

Nvidia nvcc + -ccbin of g++/icpx/clang++/nvc++ for Summit/Perlmutter/Other HPC systems

not yet, but planned to re-establish: mainline Clang CUDA (clang++ -x cuda)

not yet, but soon: NVHPC CUDA nvc++ -x cuda

(WarpX: Cray, IBM and Fujitsu support is the only thing I can currently not cover in CI.)

@ax3l impressive list. However, I think that the issue is not what SENSEI's defaults compiler flags are, but a more imprtant issue would be if Warp is attempting to make use of SENSEI's compiler flags. I think Warp should only be linking to SENSEI and that it should not be a problem that SENSEI is compiled with one compiler and set of options; and Warp a different compiler and set of options. If the cxx flags used by SENSEI are propagating to Warp then we need to fix that.

burlen · 2021-11-08T18:55:02Z

Adding -O3 is probably fine...although I am still curious as to why CMake chooses to use -O2 on some platforms. My guess is the compiler only actually goes up to level 2 optimizations and setting a higher level is simply truncated by the compiler.

I too would like to know why -O2 is the default. You're wrong about gcc only going up to -O2. -O3 certainly works and adds a number of optimizations.

It is a little annoying to set target properties...but I don't think that will be a major problem.

I'm not convinced we should pursue the target property approach right now.

burlen · 2021-11-08T18:58:42Z

@kwryankrattiger The part of this PR that makes SENSEI install relocatable should be merged. Would you please cherry pick this into a separate branch and make a PR for that? I don't think that it makes sense to hold that change up.

kwryankrattiger · 2021-11-09T17:42:41Z

@burlen I agree that this should be broken up. Relocatable, Cuda, CXX flags being added to the targets, and the fixes to the tests.

Target properties
I already pushed the per-target properties. It is not a big change and it is similar to what we are doing for CUDA. This should keep SENSEI from polluting the global CMake space so other projects can safely include it as a submodule.
Flags
Removing some of the compiler flags is relevant to ECP, arch flags are not gong to be portable, and using CMake properties is going to be more robust across the compilers and systems that will want to make use of SENSEI.

Moved this to a different PR

Use `add_compiler_options` instead of modifying the CMAKE_CXX_FLAGS directly Use CMAKE_<OPTION> to better support HPC compilers Remove micro architecture flags that are not supported by target system compilers (Fujitsu, Spock/Frontier, etc.)

kwryankrattiger · 2021-11-12T16:16:00Z

@burlen I think this should be acceptable now based on our conversations here.

I am setting the compile options via add_compile_options which is better practice than modifying the flags directly. I also removed all of the sensei_add_library and sensei_add_exectuable tags.

@ax3l I tested this and there should be no issues propagating flags set for SENSEI to projects that include it as a submodule.

burlen · 2023-03-08T18:12:10Z

CMake/build.cmake

@@ -1,7 +1,8 @@
-option(BUILD_SHARED_LIBS OFF "Build shared libraries by default")
-option(BUILD_STATIC_EXECS  OFF "Link executables statically")
+option(BUILD_SHARED_LIBS ON "Build shared libraries (default: ON)")


default to produce static libraries

kwryankrattiger marked this pull request as draft August 20, 2021 21:30

kwryankrattiger changed the title ~~Remove CXX flags and made package relocatable~~ WIP: Remove CXX flags and made package relocatable Aug 20, 2021

kwryankrattiger added the bug Something isn't working label Aug 20, 2021

burlen previously requested changes Aug 23, 2021

View reviewed changes

CMake/SENSEIConfig.cmake.in Outdated Show resolved Hide resolved

kwryankrattiger marked this pull request as ready for review August 31, 2021 19:11

kwryankrattiger changed the title ~~WIP: Remove CXX flags and made package relocatable~~ Remove CXX flags and made package relocatable Aug 31, 2021

kwryankrattiger changed the title ~~Remove CXX flags and made package relocatable~~ Remove CXX flags and make package relocatable Aug 31, 2021

burlen reviewed Sep 1, 2021

View reviewed changes

CMake/build.cmake Show resolved Hide resolved

burlen reviewed Sep 1, 2021

View reviewed changes

CMake/build.cmake Show resolved Hide resolved

burlen reviewed Sep 1, 2021

View reviewed changes

CMake/SENSEIConfig.cmake.in Outdated Show resolved Hide resolved

ax3l reviewed Sep 28, 2021

View reviewed changes