Update CP2K easyblock for CUDA version#3983
Update CP2K easyblock for CUDA version#3983pavelToman wants to merge 4 commits intoeasybuilders:developfrom
Conversation
pavelToman
changed the title
| # ensure C++ linker flags are OK | ||
| if not options.get('CXX'): | ||
| options['CXX'] = os.getenv('MPICXX') or 'mpicxx' |
There was a problem hiding this comment.
If this is required then it would be more correct to complain if the usempi toolchain opts is not set.
Just changing compiler like this won't really work unless the EC's toolchain is verified to be something that includes MPI in the first place.
I know that is a user error but this is not good practice.
| # library search paths | ||
| for libdir in (os.path.join(sirius, 'lib'), os.path.join(sirius, 'lib64')): | ||
| if os.path.isdir(libdir): | ||
| options['LDFLAGS'] += f' -L{libdir}' |
There was a problem hiding this comment.
Do we really need this? Isn't this handled by our framework already?
There was a problem hiding this comment.
Compare with the other options, like ELPA or CUDA, they aren't setting LDFLAGS
| # find cuda compute capabilities and pass it to ARCH_NUMBER | ||
| # from https://github.com/cp2k/cp2k/blob/v2025.2/exts/build_dbcsr/Makefile#L78 | ||
| get_gpu_cc = run_shell_cmd('nvidia-smi --query-gpu=compute_cap', hidden=True) | ||
| gpu_arch = get_gpu_cc.output.splitlines()[-1].strip().replace('.', '') |
There was a problem hiding this comment.
This is not correct, this sets ARCH_NUMBER to whatever compute_cap that the GPU the build is running on has instead of what the cuda-compute-capabilities configuration option specifies.
| options['DFLAGS'] += ' -D__OFFLOAD_CUDA -D__DBCSR_ACC ' | ||
| options['LIBS'] += ' -lcufft -lcudart -lnvrtc -lcuda -lcublas' | ||
| options['OFFLOAD_CC'] = 'nvcc' | ||
| options['OFFLOAD_FLAGS'] = "-O3 -g -w --std=c++11 $(DFLAGS) -Xcompiler='-fopenmp -Wall -Wextra' " |
There was a problem hiding this comment.
This should take the EB settings of the flags into account. Hardcoding is bad practice.
There was a problem hiding this comment.
and the fopenmp should not be hardcoded, it should only be used when toolchainopts openmp is true, see usage of
self.toolchain.get_flag('openmp')
below (~line 368)
| options['OFFLOAD_FLAGS'] = "-O3 -g -w --std=c++11 $(DFLAGS) -Xcompiler='-fopenmp -Wall -Wextra' " | ||
| options['OFFLOAD_TARGET'] = 'cuda' | ||
| options['ARCH_NUMBER'] = gpu_arch | ||
| options['CXX'] = 'mpicxx' |
There was a problem hiding this comment.
Again, should not hardcode to mpicxx, must use the toolchains setting, i.e. toolchainopts usempi must be true and then CXX will already be correctly set.
| options['OFFLOAD_TARGET'] = 'cuda' | ||
| options['ARCH_NUMBER'] = gpu_arch | ||
| options['CXX'] = 'mpicxx' | ||
| options['CXXFLAGS'] = '-O3 -fopenmp -g -w --std=c++14 -fPIC $(DFLAGS) $(INCS)' |
There was a problem hiding this comment.
Same here, don't hardcode options.
|
|
||
| options['DFLAGS'] += ' -D__OFFLOAD_CUDA -D__DBCSR_ACC ' | ||
| options['LIBS'] += ' -lcufft -lcudart -lnvrtc -lcuda -lcublas' | ||
| options['OFFLOAD_CC'] = 'nvcc' |
There was a problem hiding this comment.
I think there is a framework setting for this too, @boegel ?
I.e. what nvcc is supposed to be instead of hardcoding
| def configure_common(self): | ||
| """Common configuration for all toolchains""" | ||
|
|
||
| cp2k_version = LooseVersion(self.version) |
There was a problem hiding this comment.
This should be set once as self.cp2k_version probably in init and then used
| if hasattr(self.toolchain, 'MPI_FAMILY') and self.toolchain.MPI_FAMILY is not None: | ||
| known_mpi2_fams = [toolchain.MPICH, toolchain.MPICH2, toolchain.MVAPICH2, toolchain.OPENMPI, | ||
| toolchain.INTELMPI] | ||
| known_mpi2_fams = [ |
There was a problem hiding this comment.
The check for mpi should probably be done much earlier in the code, then it should also check that openmpi toochainopts is true, and then there would be no reason to change CXX/CC
| options['INCS'] += f' -I{self.libsmm_path}/include' | ||
| # library search path (keep both lib and lib64 in case) | ||
| options['LDFLAGS'] += f' -L{self.libsmm_path}/lib' |
There was a problem hiding this comment.
Should consistently use os.path.join, not hardcoded "/"
3 participants
resolves vscentrum/vsc-software-stack#297
A little bit updated PR: #3892
For: easybuilders/easybuild-easyconfigs#24454