Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake: Enforce >= C++17 for ROCm #3355

Merged
merged 2 commits into from
Jun 14, 2023

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Jun 7, 2023

Summary

Check if this adds the C++17 flags again or if there is an undocumented change in defaults that we need to manually address for AMD's Clang fork'd compilers now.

Additional background

For #3337

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • include documentation in the code and/or rst files, if appropriate

Check if this adds the C++17 flags again or if there is an
undocumented change in defaults that we need to manually
address for AMD's Clang fork'd compilers now.
@eschnett
Copy link
Contributor

eschnett commented Jun 7, 2023

No, this does not work:

#15 11.51 In file included from /cactus/src/amrex-b5b08f144134ab70e792e9b575de2d9a01f40297/Src/Base/AMReX_Random.cpp:5:
#15 11.51 In file included from /cactus/src/amrex-b5b08f144134ab70e792e9b575de2d9a01f40297/Src/Base/AMReX_Gpu.H:32:
#15 11.51 /cactus/src/amrex-b5b08f144134ab70e792e9b575de2d9a01f40297/Src/Base/AMReX_GpuContainers.H:177:65: error: expected '(' for function-style cast or type construction
#15 11.51         static_assert(std::is_same_v<value_type, out_value_type>);
#15 11.51                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^

I downloaded this commit wget https://github.com/AMReX-Codes/amrex/archive/b5b08f144134ab70e792e9b575de2d9a01f40297.tar.gz which should be the one for this pull request.

I configured like this:

    cmake \
        -DAMReX_GPU_BACKEND=HIP \
        -DAMReX_AMD_ARCH=gfx90a \
        -DAMReX_OMP=OFF \
        -DAMReX_PARTICLES=ON \
        -DAMReX_PRECISION="$precision" \
        -DBUILD_SHARED_LIBS=ON \
        -DCMAKE_BUILD_TYPE=RelWithDebInfo \
        -DCMAKE_C_COMPILER=/opt/rocm/llvm/bin/clang \
        -DCMAKE_CXX_COMPILER=/opt/rocm/llvm/bin/clang++ \
        -DCMAKE_INSTALL_PREFIX=/usr/local \
        -DCMAKE_PREFIX_PATH='/opt/rocm/lib/cmake/AMDDeviceLibs;/opt/rocm/lib/cmake/amd_comgr;/opt/rocm/lib/cmake/hip;/opt/rocm/lib/cmake/hiprand;/opt/rocm/lib/cmake/hsa-runtime64;/opt/rocm/lib/cmake/rocprim;/opt/rocm/lib/cmake/rocrand' \
        -DMPI_C_ADDITIONAL_INCLUDE_DIRS='/usr/lib/x86_64-linux-gnu/openmpi/include/openmpi;/usr/lib/x86_64-linux-gnu/openmpi/include' \
        -DMPI_C_LIB_NAMES='mpi;open-rte;open-pal;hwloc' \
        -DMPI_CXX_ADDITIONAL_INCLUDE_DIRS='/usr/lib/x86_64-linux-gnu/openmpi/include/openmpi;/usr/lib/x86_64-linux-gnu/openmpi/include' \
        -DMPI_CXX_LIB_NAMES='mpi_cxx;mpi;open-rte;open-pal;hwloc' \
        -DMPI_hwloc_LIBRARY=/lib/x86_64-linux-gnu/libhwloc.so \
        -DMPI_mpi_LIBRARY=/usr/lib/x86_64-linux-gnu/openmpi/lib/libmpi.so \
        -DMPI_mpi_cxx_LIBRARY=/usr/lib/x86_64-linux-gnu/openmpi/lib/libmpi_cxx.so \
        -DMPI_open-pal_LIBRARY=/lib/x86_64-linux-gnu/libopen-pal.so \
        -DMPI_open-rte_LIBRARY=/lib/x86_64-linux-gnu/libopen-rte.so \
        .. && \

@WeiqunZhang
Copy link
Member

Does target_compile_options(amrex_${D}d PUBLIC $<$<COMPILE_LANGUAGE:CXX>:-std=c++17>) work?

I think cmake is trying to be smart. Its logic might be that since the official Clang 16 supports C++17 by default and you ask for the C++17 feature so I don't have to do anything.

@ax3l
Copy link
Member Author

ax3l commented Jun 12, 2023

Yes, I agree that this is probably exactly what is happening. Looks like the ROCm clang fork changed the CXX default of Clang -.-

CMake deduplicates the std C++17 flags in the assumption that the default did not change to vanilla LLVM.
@ax3l
Copy link
Member Author

ax3l commented Jun 12, 2023

Fun fact: hipcc did the same weirdness, one of the reasons I did not use it anymore
https://gist.github.com/ax3l/53db9fa8a4f4c21ecc5c4100c0d93c94

@ax3l
Copy link
Member Author

ax3l commented Jun 13, 2023

@eschnett does this work for you now? :)

@eschnett
Copy link
Contributor

@ax3l this works. AMReX builds with ROCM 5.5.1.

@WeiqunZhang WeiqunZhang merged commit 41a6700 into AMReX-Codes:development Jun 14, 2023
64 checks passed
@ax3l ax3l deleted the fix-cmake-rocm-5.5-std17 branch June 14, 2023 17:28
@ax3l
Copy link
Member Author

ax3l commented Jun 14, 2023

Ok, let's now triage this with AMD as well.

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

Successfully merging this pull request may close these issues.

None yet

3 participants