Skip to content

Issue 155 New Unit Test for Field #156

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

LinhWuerzburger
Copy link
Contributor

added two tests that control the data pointer after a change in the data field

@LinhWuerzburger LinhWuerzburger linked an issue Sep 1, 2021 that may be closed by this pull request
@LinhWuerzburger LinhWuerzburger force-pushed the issue_155_new_unit_test_for_field branch from cb7f66f to c620e5d Compare September 1, 2021 12:28
@LinhWuerzburger
Copy link
Contributor Author

LinhWuerzburger commented Sep 1, 2021

we may consider removing the Field::swap method
as it is currently implemented it can cause problems for the GPU implementation.

For example:
Field a with pointer 123 to its field and
Field b with pointer 345 to its field
both send their field to the GPU through data_pointer_a = 123, data_pointer_b = 345
GPU stores 123 as well 345
Field::swap(a, b)
now it looks like this:
a with pointer 345
b with pointer 123
data_pointer_a = 123
data_pointer_b = 345

if you know forget to swap data_pointer_a and data_pointer_b as well it is doomed to fail. But if the developer has to use swap it is clear what happens. of course, he is free to look into the Field::swap method but I think that is an easy source for an error when you are not fully aware of what happens in the Field::swap method. Especially because you are calling the function with the Field objects and not the data pointer.
At least a warning is needed as part of the function's documentation, in case the function is not removed.

return *this;
}

Field &operator*=(const real x) {
#pragma acc parallel loop independent present(this->data[:m_size]) async
#pragma acc parallel loop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't we need a present clause here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same matter in lines 90 and 73

Copy link
Contributor Author

Choose a reason for hiding this comment

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

independent is also important to tell the compiler that the loop can be freely parallelise

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.

Unit test: new unit test for Field
2 participants