Skip to content

cmake update for main VASP version#1

Open
the-hampel wants to merge 6 commits intoAdhocMan:mainfrom
the-hampel:cmake_integration
Open

cmake update for main VASP version#1
the-hampel wants to merge 6 commits intoAdhocMan:mainfrom
the-hampel:cmake_integration

Conversation

@the-hampel
Copy link
Copy Markdown

Hi @AdhocMan ,

I made various changes to the cmake files to work for all supported platforms. These are quite drastic changes, but I thought I would like to update you on the progress I made and how the version looks that we merge into the master branch of VASP.

One of the most important changes is here that I am decided to preprocess all fortran files first manually to solve any dependency issues. This works now very reliably.

If you have any comments on these I am happy to incorporate any suggestions you have. If you see any problems let me also now.

And thank you again for your work on this. This was a great help.

Copy link
Copy Markdown
Owner

@AdhocMan AdhocMan left a comment

Choose a reason for hiding this comment

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

Thanks for providing these changes!
It appears to work fine in our GH200 cluster environment, including parallel compilation with make.
I have added a few comments on some parts.

Comment thread cmake/FindSCALAPACK.cmake Outdated
Comment thread cmake/FindSCALAPACK.cmake Outdated
Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
#Get CUDA compute capability
set(OUTPUTFILE ${CMAKE_CURRENT_BINARY_DIR}/cuda_script) # No suffix required
set(CUDAFILE ${CMAKE_CURRENT_SOURCE_DIR}/cmake/check_cuda.cu)
execute_process(COMMAND nvcc -lcuda ${CUDAFILE} -o ${OUTPUTFILE})
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'd strongly recommend not to do this detection logic.
On some clusters a GPU may not be available at compilation and this will fail. Additionally, the nvcc executable may not be in PATH, so this would have to be looked for through enable_language(CUDA).
The detection of the default architecture is already handled by setting CMAKE_CUDA_ARCHITECTURES to native, since -gpu=ccnative lets nfortran do the detection.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes I see what you mean. I found this solution somewhere in the internet and later discovered that cmake can detect cuda by itself with check_language(CUDA) . Do you know if this command actually also checks that nvcc is working? Similar to how it is done with gcc? But anyway I can drop this. I see the point that test compiling via nvcc might be okay, even if no GPU is present, but executing is a problem. I will remove that part.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

see b801e20

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Using the check_language feature relates to the broader question of whether you'd want to set options automatically. One could apply similar logic to other features like OpenMP. Personally, I'd tend to prefer fixed settings.
But generally speaking, there should always be a way to disable any automatic option selection, since it's much better for package managers like spack to fail if an selected option does not work. Using set(VASP_CUDA ON) will always override the user provided setting.
If there is a need for default settings for certain use cases, maybe have a look at https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html. This should allow to define sets of options to be used together, for example when building with CUDA.

Also, adding the check if(${CMAKE_CUDA_ARCHITECTURES} STREQUAL "native") is unessary, since the flag will already be correctly set through

    set(VASP_CUDA_ARCH ${CMAKE_CUDA_ARCHITECTURES})
    list(POP_FRONT VASP_CUDA_ARCH VASP_FIRST_ARCH)
    set(VASP_GPU_FLAG "-gpu=cc${VASP_FIRST_ARCH}")

Comment thread CMakeLists.txt Outdated
Comment thread cmake/sources_and_flags_options.cmake Outdated
Comment thread testsuite/CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
Comment thread testsuite/CMakeLists.txt Outdated
@the-hampel
Copy link
Copy Markdown
Author

Thanks for taking such detailed look at the PR @AdhocMan. These are all very helpful and I tried to address all of them. I answered to all comments separately above and incorporated the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants