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

Allow using float shape functions #9104

Merged
merged 1 commit into from Dec 3, 2019

Conversation

masterleinad
Copy link
Member

Previously, we were not able to compile with float as number type since global_co_shape_gradients/global_shape_gradients/global_shape_values were double arrays.
This pull request fixes the issue by duplicating the storage for float.

@masterleinad masterleinad changed the title Allos using float shape functions Allow using float shape functions Nov 27, 2019
Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

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

Extending to float is a good idea. However, lets first discuss on the style (especially from the background that we might want in the future multiple shape_values; e.g. different polynomial degrees, over-integration).

Comment on lines +51 to +57
global_shape_values_d[(max_elem_degree + 1) * (max_elem_degree + 1)];
__constant__ float
global_shape_values_f[(max_elem_degree + 1) * (max_elem_degree + 1)];
Copy link
Member

Choose a reason for hiding this comment

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

This looks ugly. Any chance to create a struct like:

template<typename T>
struct ShapeData
{
    T global_shape_values[(max_elem_degree + 1) * (max_elem_degree + 1)];
    T global_shape_gradients[(max_elem_degree + 1) * (max_elem_degree + 1)];
    T global_co_shape_gradients[(max_elem_degree + 1) * (max_elem_degree + 1)];
}

In the long term, it would be nice to be able to write __constant__ ShapeData<Number, degree, q_points> or s.th. similar. It would be nice to have as much flexibility as in the case of the CPU version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a bad suggestion even if helps only slightly. We still need separate global __constant__ variables (function scope static variables are also not allowed) and this is also the reason that we can't templatize on degree and n_q_points in a meaningful way. These arrays have to exist independent of the polynomial degree. Of course, we could create separate arrays, but that would only waste space.

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 give up on improving this. Accessing __constant__ memory seems to be really brittle. If you pass a pointer instead of a reference to a C-style array or try to use a struct for structuring the data better, you always get an invalid device symbol error.

Copy link
Member

Choose a reason for hiding this comment

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

If you pass a pointer instead of a reference to a C-style

I also tried that out some time ago; I had the same conclusion :( Thanks for trying out the struct approach! Let's merge!

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

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

Thanks!

@masterleinad
Copy link
Member Author

I had to discard the struct again to make it actually work.

@kronbichler kronbichler merged commit 8cb1e14 into dealii:master Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants