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.

Sign up for GitHub

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

Jump to bottom

Require C++17 support #1603

Merged
merged 13 commits into from
Jul 8, 2024
Merged

Require C++17 support #1603

merged 13 commits into from
Jul 8, 2024

Conversation

upsj
Copy link
Member

@upsj upsj commented May 1, 2024

This requires C++17 support and removes the workarounds we had in place before

@upsj upsj added the 1:ST:ready-for-review This PR is ready for review label May 1, 2024
@upsj upsj requested a review from a team May 1, 2024 13:34
@upsj upsj self-assigned this May 1, 2024
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. reg:documentation This is related to documentation. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. reg:example This is related to the examples. type:solver This is related to the solvers type:preconditioner This is related to the preconditioners mod:hip This is related to the HIP module. type:factorization This is related to the Factorizations type:reordering This is related to the matrix(LinOp) reordering mod:dpcpp This is related to the DPC++ module. labels May 1, 2024
@upsj
Copy link
Member Author

upsj commented May 1, 2024

This also means bumping GCC to version 7, Clang to version 5 and CUDA to version 11

This was referenced May 1, 2024
Merged
Merged
@tcojean tcojean added this to the Ginkgo 1.9.0 milestone May 3, 2024
MarcelKoch
MarcelKoch requested changes Jun 14, 2024
View reviewed changes
include/ginkgo/core/log/profiler_hook.hpp Show resolved Hide resolved
README.md Show resolved Hide resolved
@MarcelKoch MarcelKoch mentioned this pull request Jun 27, 2024
Open
2 tasks
yhmtsai
yhmtsai requested changes Jun 28, 2024
View reviewed changes
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

another question: Maybe I miss the discussion, do we drop the cuda 10.1/10.2 support?

.gitlab-ci.yml Outdated Show resolved Hide resolved
.gitlab-ci.yml
@@ -291,40 +159,6 @@ test/cuda110/nompi/clang/cuda/release/static:
needs: [ "build/cuda110/nompi/clang/cuda/release/static" ]


build/cuda110/nompi/intel/cuda/debug/static:
Copy link
Member

Choose a reason for hiding this comment

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

why is it removed?

Copy link
Member Author

@upsj upsj Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nvcc with icpc as the host compiler can't compile a simple C++17 header like <variant>, so I would say that qualifies as not properly supporting C++17, see https://gitlab.com/ginkgo-project/ginkgo-public-ci/-/jobs/7265870465

README.md Show resolved Hide resolved
core/test/accessor/CMakeLists.txt
target_compile_features("${TEST_TARGET_NAME}" PUBLIC cxx_std_14)
target_compile_features("${TEST_TARGET_NAME}" PUBLIC cxx_std_17)
Copy link
Member

Choose a reason for hiding this comment

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

only this changes from 14 to 17, but others are just removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

the others are all implied by the PUBLIC property on the ginkgo target

thoasm
thoasm requested changes Jul 2, 2024
View reviewed changes
Copy link
Member

@thoasm thoasm left a comment

Choose a reason for hiding this comment

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

Please also change the include/ginkgo/core/base/std_extensions.hpp (for example, remove the make_void and change the implementation of uncaught_exception()).
I would also like to know why the tests/pipelines were dropped.

examples/custom-matrix-format/CMakeLists.txt Outdated Show resolved Hide resolved
@upsj
Copy link
Member Author

upsj commented Jul 4, 2024

@thoasm changes in std_extensions would technically break interface, so I wanted to avoid them

@thoasm
Copy link
Member

thoasm commented Jul 5, 2024

@upsj I'm not suggesting to drop the functions/structures completely, but you can implement xstd.:uncaught_exception with std::uncaught_exception. The same with xstd.:void_t. We did this for all the other members of the xstd namespace.
The make_void helper I was talking about is in detail, so dropping it is fine.

@upsj upsj force-pushed the c++17 branch 2 times, most recently from 4fc0bce to b59f135 Compare July 5, 2024 10:36
thoasm
thoasm approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@thoasm thoasm left a comment

Choose a reason for hiding this comment

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

LGTM!

yhmtsai
yhmtsai reviewed Jul 8, 2024
View reviewed changes
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

Because it removes the check for cuda_arch 35, need to add this back to other jobs.
I think setting the standard through the property is more clear.
For example, hip_std_17 is not searchable in cmake 3.21.7 (the minimal requirement for hip module).

cmake/build_helpers.cmake Show resolved Hide resolved
yhmtsai
yhmtsai approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

only needs some tests for cuda arch 35

@upsj upsj added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Jul 8, 2024
upsj added 2 commits July 8, 2024 16:48
@upsj
remove warning flags
4cc9249
@upsj
move to a compatible nvcc/clang combination
f570559 
nvcc with clang++ host compiler seems incompatible with libstdc++-10
@upsj
Copy link
Member Author

upsj commented Jul 8, 2024

@yhmtsai I needed to add a new pipeline, since all remaining ones both build and test, so they are restricted to our available GPUs. compute capabilities 3.5 - 5.0 have been deprecated in CUDA 11, so I'm using 5.2 for the build. We don't have any GPUs below compute capability 6.0 anyways.

@upsj upsj merged commit caa373d into develop Jul 8, 2024
12 of 14 checks passed
@upsj upsj deleted the c++17 branch July 8, 2024 22:04
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@nbeams nbeams mentioned this pull request Aug 15, 2024
Open
61 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcelKoch MarcelKoch MarcelKoch approved these changes

@thoasm thoasm thoasm approved these changes

@yhmtsai yhmtsai yhmtsai approved these changes

Assignees

@upsj upsj

Labels
1:ST:ready-to-merge This PR is ready to merge. 1:ST:run-full-test mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:dpcpp This is related to the DPC++ module. mod:hip This is related to the HIP module. reg:build This is related to the build system. reg:documentation This is related to documentation. reg:example This is related to the examples. reg:testing This is related to testing. type:factorization This is related to the Factorizations type:preconditioner This is related to the preconditioners type:reordering This is related to the matrix(LinOp) reordering type:solver This is related to the solvers
Projects
None yet
Milestone
Ginkgo 1.9.0
Development

Successfully merging this pull request may close these issues.
6 participants
@upsj @thoasm @MarcelKoch @yhmtsai @tcojean @ginkgo-bot