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

[SYCL] Move an accessor pointer to global_device space #2044

Merged
merged 8 commits into from
Jul 10, 2020

Conversation

MrSidims
Copy link
Contributor

@MrSidims MrSidims commented Jul 6, 2020

With this patch an accessor pointer to global buffer is moved from
global space to global_device space. That is done to distinguish this
pointer from those USM pointers, that are allocated global space or
global_host space, in compile time.

In addition to this change there are added explicit conversion operator
from global_device to global space for multi_ptr class and implicit
convertion for atomic class from global_device for global space.
The last change isn't covered by specification published here:
#1840 , but is required to pass
atomic_api CTS.

@MrSidims MrSidims requested a review from a team as a code owner July 6, 2020 15:56
@MrSidims MrSidims requested review from v-klochkov and mlychkov July 6, 2020 15:56
@MrSidims MrSidims force-pushed the private/MrSidims/MoveAccessors branch from ee0ad94 to 0bbd843 Compare July 6, 2020 17:59
MrSidims added 4 commits July 8, 2020 00:25
With this patch an accessor pointer to global buffer is moved from
global space to global_device space. That is done to distinguish this
pointer from those USM pointers, that are allocated global space or
global_host space, in compile time.

In addition to this change there are added explicit conversion operator
from global_device to global space for multi_ptr class and implicit
convertion for atomic class from global_device for global space.
The last change isn't covered by specification published here:
intel#1840 , but is required to pass
atomic_api CTS.

Signed-off-by: Dmitry Sidorov <[email protected]>
Signed-off-by: Dmitry Sidorov <[email protected]>
Signed-off-by: Dmitry Sidorov <[email protected]>
Signed-off-by: Dmitry Sidorov <[email protected]>
@MrSidims MrSidims force-pushed the private/MrSidims/MoveAccessors branch from 6fcfbcc to f6d8bdf Compare July 7, 2020 22:10
@MrSidims MrSidims requested a review from v-klochkov July 8, 2020 11:37
@MrSidims
Copy link
Contributor Author

MrSidims commented Jul 8, 2020

@intel/llvm-reviewers-runtime ping

Signed-off-by: Dmitry Sidorov <[email protected]>
v-klochkov
v-klochkov previously approved these changes Jul 8, 2020
Copy link
Contributor

@v-klochkov v-klochkov left a comment

Choose a reason for hiding this comment

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

Not a request to change, but it would be nice to fix it in my opinion.
The 'atomic' class seems to be ripped by those 'private', 'public' keywords already and your fix added more to it.

With your fix the class has: private: public: private: public: private.
I suggest moving those new private getter methods to the line 336

@MrSidims
Copy link
Contributor Author

MrSidims commented Jul 8, 2020

Not a request to change, but it would be nice to fix it in my opinion.
The 'atomic' class seems to be ripped by those 'private', 'public' keywords already and your fix added more to it.

With your fix the class has: private: public: private: public: private.
I suggest moving those new private getter methods to the line 336

I don't mind to change it, here I was just following Robert C. Martin "Clean code" advise to order methods in a way so readers have no need to scroll the code too much. What do you think?

Signed-off-by: Dmitry Sidorov <[email protected]>
@MrSidims
Copy link
Contributor Author

MrSidims commented Jul 8, 2020

Not a request to change, but it would be nice to fix it in my opinion.
The 'atomic' class seems to be ripped by those 'private', 'public' keywords already and your fix added more to it.
With your fix the class has: private: public: private: public: private.
I suggest moving those new private getter methods to the line 336

I don't mind to change it, here I was just following Robert C. Martin "Clean code" advise to order methods in a way so readers have no need to scroll the code too much. What do you think?

Well, actually, now I can even remove this method.

@v-klochkov
Copy link
Contributor

I don't mind to change it, here I was just following Robert C. Martin "Clean code" advise to order methods in a way so readers have no need to scroll the code too much. What do you think?

In the process of answer to this question I was going to still suggest moving the methods to the place where the field declared. Then I realized that atomic::getPtr() is used only inside atomic class. This makes that getter method redundant, in my opinion.

I suppose you added it to public section before adding that "friend" class and that was the purpose of the getter method. After adding "friend" statement there is no big need in that getter.
In the constructor you can just write: "Ptr = RHS.Ptr;" instead of "Ptr = RHS.getPtr();"

@MrSidims
Copy link
Contributor Author

MrSidims commented Jul 8, 2020

I don't mind to change it, here I was just following Robert C. Martin "Clean code" advise to order methods in a way so readers have no need to scroll the code too much. What do you think?

In the process of answer to this question I was going to still suggest moving the methods to the place where the field declared. Then I realized that atomic::getPtr() is used only inside atomic class. This makes that getter method redundant, in my opinion.

I suppose you added it to public section before adding that "friend" class and that was the purpose of the getter method. After adding "friend" statement there is no big need in that getter.
In the constructor you can just write: "Ptr = RHS.Ptr;" instead of "Ptr = RHS.getPtr();"

Yeap, I've just removed it :)

Signed-off-by: Dmitry Sidorov <[email protected]>
v-klochkov
v-klochkov previously approved these changes Jul 8, 2020
Signed-off-by: Dmitry Sidorov <[email protected]>
@bader bader merged commit bc42582 into intel:sycl Jul 10, 2020
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.

5 participants