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

Bump minimum C++ version to C++17, use C++ 17 by default #206

Merged
merged 1 commit into from
Mar 5, 2020

Conversation

illuhad
Copy link
Collaborator

@illuhad illuhad commented Feb 23, 2020

This makes hipSYCL use C++17 by default. This also raises the minimum supported C++ version to C++17. This PR

  • will allow users to use C++ 17 constructs in their code by default
  • will allow us to use C++ 17 in the hipSYCL headers.

@illuhad
Copy link
Collaborator Author

illuhad commented Feb 24, 2020

@sbalint98 please confirm that this doesn't break the packages.

@keryell
Copy link
Contributor

keryell commented Feb 27, 2020

The more modern, the happier I am! :-)

@illuhad
Copy link
Collaborator Author

illuhad commented Feb 28, 2020

@keryell I thought that you might like this. A gift just for you ;)

@jeffhammond
Copy link
Contributor

How many days before @keryell demands bumping it to C++20? The ink has almost dried on that one! 😜

@keryell
Copy link
Contributor

keryell commented Feb 28, 2020

How many days before @keryell demands bumping it to C++20? The ink has almost dried on that one! stuck_out_tongue_winking_eye

If I ask, it will not be a gift anymore. I love to be surprised... ;-)

@mrzv
Copy link
Contributor

mrzv commented Feb 29, 2020

Would this mean that hipSYCL becomes unusable with compilers that don't support C++17? What's the rush to do this?

@jeffhammond
Copy link
Contributor

jeffhammond commented Feb 29, 2020 via email

@mrzv
Copy link
Contributor

mrzv commented Feb 29, 2020

Oh, I don't doubt that C++17 is better, but I'm concerned how widely it is supported. If all goes as planned, I'll want to run hipSYCL code on Summit later this year. It's not clear to me how well C++17 is supported there. Their docs don't seem to mention it at all.

@jeffhammond
Copy link
Contributor

jeffhammond commented Feb 29, 2020 via email

@mrzv
Copy link
Contributor

mrzv commented Feb 29, 2020

I saw a module for GCC 9.1.0 there. That's not my concern. The bigger issue is what constraints the simulation folks have. (The hipSYCL code would be some analysis running alongside a simulation.)

@jeffhammond
Copy link
Contributor

jeffhammond commented Feb 29, 2020 via email

@illuhad
Copy link
Collaborator Author

illuhad commented Feb 29, 2020

Would this mean that hipSYCL becomes unusable with compilers that don't support C++17?

Yes, once we start using C++17 features in the headers.

What's the rush to do this?

I wouldn't consider it a rush. This change is because of

  • Alignment with other implementations. Most are already at C++17 or even beyond, or are in the process of also moving to C++17
  • Alignment with what I personally believe is likely to end up in the next version of the SYCL spec (because of first point).

I think that it is highly likely that we will have to do this change anyway in the near future to stay up to date with the rest of the SYCL world.

Note that building hipSYCL already has a dependency on clang >= 8, which is used for compiling GPU code. So, at least for GPU we are guaranteed to have a C++17 compiler. I understand that you are on Power9, possibly xlc++. That compiler already has some shaky C++14 support, so it could be that even currently it suddenly breaks (if it works at the moment at all) if we use some C++14 features that it happens to not support... Please let me know what your constraints are.

If you are not bound to xlc++ and the question is merely how to integrate C++17 hipSYCL with the build system of the simulation code, I believe there's a solution. Maybe we could add an option to syclcc that causes it to silently replace -std=c++14 passed by the build system with -std=c++17 or something like that. In any case, as @jeffhammond pointed out, I'm sure there's a solution.

@mrzv
Copy link
Contributor

mrzv commented Feb 29, 2020

@illuhad Got it. I checked, and the simulation is using PGI 19.10 on Summit, which supports C++17. And you are also right: my main interest in hipSYCL is for the GPU code, which should be fine because of the clang requirement. So the main issue is going to be to figure out the build system, but I think that's orthogonal to this PR.

@illuhad illuhad merged commit 17d0ec7 into master Mar 5, 2020
@illuhad illuhad deleted the require-cpp17-by-default branch March 5, 2020 14:17
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.

None yet

4 participants