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

Move matrix free classes from CUDAWrappers namespace to Portable namespace #16497

Merged
merged 3 commits into from Feb 12, 2024

Conversation

Rombur
Copy link
Member

@Rombur Rombur commented Jan 18, 2024

This is a proposal to move the matrix free classes out of the CUDAWrappers namespace to a new Portable namespace. The current namespace doesn't make sense anymore because the current code works fine on AMD GPUs. I am open to a different name for the new namespace if someone as a better idea. There is the issue on how to deprecated the old namespace. nvcc seems to have an issue with the deprecated attribute for a namespace.

Copy link
Member

@marcfehling marcfehling left a comment

Choose a reason for hiding this comment

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

Looks good to me. I also don't have a better name for the namespace, but I do like that your choice is 4 characters shorter, which gets rid of some linebreaks in clang-format :)

Would you please also add a changelog entry to incompatibilities?

FEEvaluation<dim, fe_degree, n_q_points_1d, n_components_, Number>::
n_q_points;
#endif
using namespace Portable;
Copy link
Member

Choose a reason for hiding this comment

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

That's clever. Is there also a way to effectively deprecate the whole CUDAWrappers namespace?

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 problem is that nvcc doesn't like when I deprecate the namespace. With the others compilers I can do
namespace DEAL_II_EARLY_DEPRECATION CUDAWrappers and it works like expected. We could only emit the deprecation warning when people use a compiler other than nvcc but I would expect that almost everybody that uses this class also uses nvcc.

Copy link
Member

Choose a reason for hiding this comment

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

What does "nvcc doesn't like when I deprecate the namespace" exactly mean?
Is the attribute just not recognized and the compiler warns about that (like in https://godbolt.org/z/Mf3444sP9 which only recognizes the attribute from nvcc 11.1.0 on) or is there a different issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the compiler complains that attribute is not recognized.

Copy link
Member

Choose a reason for hiding this comment

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

We should just do a version check then since it seems to work from nvcc 11.1.0 on.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that the issue is due to nvcc version. The CI fails with nvcc 11.8. I think that the problem is the underlying gcc compiler. The CI uses gcc 9 which had a bug when using [[deprecated]] with a namespace (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79817)

@masterleinad masterleinad self-requested a review January 24, 2024 17:09
@Rombur Rombur force-pushed the rename_cudawrappers branch 2 times, most recently from 60b14c7 to e37a9cc Compare February 9, 2024 18:07
@Rombur
Copy link
Member Author

Rombur commented Feb 9, 2024

/rebuild

@Rombur
Copy link
Member Author

Rombur commented Feb 12, 2024

Unless someone has an objection, I will merge this tomorrow.

@masterleinad masterleinad merged commit f9615bd into dealii:master Feb 12, 2024
15 checks passed
@Rombur Rombur deleted the rename_cudawrappers branch February 13, 2024 20:16
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

4 participants