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

C++23 support #838

Merged
merged 16 commits into from
Jul 8, 2024
Merged

C++23 support #838

merged 16 commits into from
Jul 8, 2024

Conversation

schnellerhase
Copy link
Contributor

No description provided.

@schnellerhase
Copy link
Contributor Author

Hefty changes necessary to adapt to mdspan interface change from (...) to [...].

@schnellerhase schnellerhase force-pushed the c++23 branch 2 times, most recently from 56863dc to 146f5ce Compare July 2, 2024 15:05
@jhale
Copy link
Member

jhale commented Jul 3, 2024

Let's assess this now before wasting time moving forward on a patch we cannot merge.

@schnellerhase
Copy link
Contributor Author

Sounds good, so the local build with a GCC version 13.2 and the macos CI build seem to be passing. Besides that it could become tricky to address the absence of a mdpsan implementation with c++23 ready compilers. Even trickier is the case of nof c++23 support and neither mdspan - i.e. we can probably not make us of [...] in these cases.

@jhale
Copy link
Member

jhale commented Jul 3, 2024

Sounds promising - GCC 11.4.1 on RedHat 9-type distributions is going to be the lower bound. There are quite a few errors there - what do you think?

Regarding mdspan, can't we keep on using the Kokkos one in all cases for now? Even if there is one built in.

@schnellerhase
Copy link
Contributor Author

schnellerhase commented Jul 3, 2024

They seem to be all caused by some static assert/requirement. Not sure why this is a problem in these cases and otherwise not though.

Will try with the Kokkos mdspan - also will check if we are up to date.

@schnellerhase
Copy link
Contributor Author

How was the mdspan.hpp file generated? I saw the make_single_header.py file in Kokkos mdspan repo, but not sure in which config this produces the here present mdspan.hpp -> tried python make_single_header.py include/mdspan.hpp > mdspan.hpp but that does not include the mdarray.hpp which seems to be part of it here as well.

@jhale
Copy link
Member

jhale commented Jul 3, 2024

Kokkos mdspan does claim to support GCC 11 with C++23 with Wall, see their README.

@garth-wells has done the mdspan header copy in the past.

@garth-wells
Copy link
Member

Kokkos mdspan does claim to support GCC 11 with C++23 with Wall, see their README.

@garth-wells has done the mdspan header copy in the past.

See https://github.com/kokkos/mdspan/tree/single-header.

@schnellerhase
Copy link
Contributor Author

OK we have 2 builds passing, macos and the dolfinx integration are working (at least the basix build)

@schnellerhase
Copy link
Contributor Author

Have you guys maybe got an idea what may be the cause for the very weird error I'm getting with this small snippet to test why the ubuntu builds are failing? Note that I can access with a std::array as indices the mdspan properly, but when using the [] operator the assert fails. Testing with the pass function yields the correct size assertion however....

This is observed with gcc version 11.4

#include "mdspan.hpp"

template <typename T, std::size_t D>
using mdspan_t = MDSPAN_IMPL_STANDARD_NAMESPACE::mdspan<T, MDSPAN_IMPL_STANDARD_NAMESPACE::dextents<std::size_t, D>>;

template<class...IndexType>
void pass(IndexType... indices)
{
    static_assert(sizeof...(IndexType) == 2);
}

void test()
{
    mdspan_t<double, 2> span;
    static_assert(span.rank() == 2);

    std::array<std::size_t, 2> indices = {0, 0};
    static_assert(indices.size() == 2);
    span[indices];

    pass(0, 0);

    static_assert(std::is_convertible_v<decltype((0,0)), mdspan_t<double, 2>::index_type>);
    static_assert(std::is_nothrow_constructible_v<mdspan_t<double, 2>::index_type, decltype((0,0))>);

    span[0, 0]; // but this fails with (rank() == sizeof...(SizeTypes)) evaluating to False
}

@schnellerhase
Copy link
Contributor Author

schnellerhase commented Jul 5, 2024

Okay a small update, I experimented with not updating the () to [] operator, by explicitly enforcing the former one. This seems to work, see schnellerhase#1 (up to the intel compiler at least - gotta figure out whats going wrong there, because in principle I think they are c++23 ready). So I suggest we remove the upgrading to the [] operator from this PR for now and figure out whats going on there in another later one?

@schnellerhase schnellerhase force-pushed the c++23 branch 2 times, most recently from 2e527cf to 8dc7685 Compare July 5, 2024 09:38
@schnellerhase
Copy link
Contributor Author

schnellerhase commented Jul 5, 2024

Not enforcing the standard fixes the cmake error with the intel compiler.

It looks like they are not fully ready to support the iso standard for c++23 so it gets a different flag - which I assume causes the problem: https://www.intel.com/content/www/us/en/docs/dpcpp-cpp-compiler/developer-guide-reference/2023-0/std-qstd.html

I am not aware of how I could hint cmake to use this flag, and turning off the standard enforcement also is no good solution I imagine. How should we ago about this?

@jhale
Copy link
Member

jhale commented Jul 5, 2024

Would be worth checking with a very modern version of Cmake installed in the CI to see if it passes the right flag. Otherwise you can detect the Intel Compiler and pass the flag manually.

@schnellerhase
Copy link
Contributor Author

Newer cmake does the trick! I used version 3.30, are there any preferences on which version we should use in this CI?

cpp/basix/mdspan.hpp Outdated Show resolved Hide resolved
.github/workflows/intel.yml Show resolved Hide resolved
.github/workflows/intel.yml Show resolved Hide resolved
demo/cpp/demo_create_and_tabulate/CMakeLists.txt Outdated Show resolved Hide resolved
python/CMakeLists.txt Outdated Show resolved Hide resolved
@jhale
Copy link
Member

jhale commented Jul 7, 2024

As an aside, MSVC also doesn't support P2128R6 Multidimensional subscript operator yet so this is the right way to go for now.

@jhale
Copy link
Member

jhale commented Jul 7, 2024

And PR2128R6 was only added in GCC 12 according to this RedHat article https://developers.redhat.com/articles/2022/04/25/new-c-features-gcc-12#c__23_features

@schnellerhase
Copy link
Contributor Author

And PR2128R6 was only added in GCC 12 according to this RedHat article https://developers.redhat.com/articles/2022/04/25/new-c-features-gcc-12#c__23_features

That could explain my findings with the code snippet before - not sure how it explains the assertion going off though.

@schnellerhase schnellerhase changed the title C++23 (experimental) support C++23 support Jul 8, 2024
@jhale jhale added this pull request to the merge queue Jul 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 8, 2024
@schnellerhase
Copy link
Contributor Author

schnellerhase commented Jul 8, 2024

Is the website failure caused by the here introduced changes or does this originate from somewhere else?

@jhale
Copy link
Member

jhale commented Jul 8, 2024

Something to do with documentation build - I don't fully understand the error (some sort of deprecation warning).

@schnellerhase
Copy link
Contributor Author

Yeah, very weird, I'm not even sure it's the deprecation warning though. Never used any of the packages that get used for the website construction, so I'm not sure how to go about it.

@schnellerhase
Copy link
Contributor Author

Should be fixed with FEniCS/web#187.

@jhale jhale added this pull request to the merge queue Jul 8, 2024
Merged via the queue into FEniCS:main with commit dcade4b Jul 8, 2024
14 checks passed
@schnellerhase schnellerhase deleted the c++23 branch July 8, 2024 15:39
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.

3 participants