Skip to content

[SYCL] Changing all PI APIs to the standard format of returning pi_result #776

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 14, 2019
Merged

[SYCL] Changing all PI APIs to the standard format of returning pi_result #776

merged 4 commits into from
Nov 14, 2019

Conversation

garimagu
Copy link
Contributor

@garimagu garimagu commented Oct 30, 2019 •
edited
Loading

Please review only the last commit. The other commits are from #708

Signed-off-by: Garima Gupta [email protected]

@romanovvlad
Copy link
Contributor

@garimagu
What is the reason of changing function signatures?
This change kills opportunity to have OpenCL function pointer in PI dispatch table to avoid redundant proxy.

@garimagu
Copy link
Contributor Author

@garimagu
What is the reason of changing function signatures?
This change kills opportunity to have OpenCL function pointer in PI dispatch table to avoid redundant proxy.

But only for a few APIs.
This change will standardize the call sites, and make calling and tracing of each function easier.
The next few patches are:

  • replace the PI_CALLs with PI_TRACE, and
  • replace a function call with the call to the function pointer at a specific offset in the function table. The change makes all that code very easy to implement and understand.

@garimagu garimagu force-pushed the private/garimagu/pi_api_piresult branch from 6910497 to 51f066a Compare November 5, 2019 20:03
@garimagu
Copy link
Contributor Author

garimagu commented Nov 5, 2019

Please review only the last commit. The other commits are from #708

@garimagu garimagu force-pushed the private/garimagu/pi_api_piresult branch from 51f066a to 0868f4f Compare November 5, 2019 23:13
@asavonic
Copy link
Contributor

asavonic commented Nov 6, 2019

This change kills opportunity to have OpenCL function pointer in PI dispatch table to avoid redundant proxy.

@romanovvlad, does this have any impact on performance?

@romanovvlad
Copy link
Contributor

This change kills opportunity to have OpenCL function pointer in PI dispatch table to avoid redundant proxy.

@romanovvlad, does this have any impact on performance?

Yes, it can have performance impact, however I don't think it will be significant.

smaslov-intel
smaslov-intel previously approved these changes Nov 7, 2019
View reviewed changes
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing this!

@garimagu garimagu force-pushed the private/garimagu/pi_api_piresult branch from 0868f4f to 83b1c51 Compare November 8, 2019 18:09
@garimagu
Copy link
Contributor Author

garimagu commented Nov 8, 2019

I rebased it because of conflicts. Please approve again.
@romanovvlad @smaslov-intel Please merge.

smaslov-intel
smaslov-intel previously approved these changes Nov 8, 2019
View reviewed changes
@garimagu garimagu force-pushed the private/garimagu/pi_api_piresult branch from 83b1c51 to 53f411c Compare November 8, 2019 20:01
@garimagu garimagu force-pushed the private/garimagu/pi_api_piresult branch from 53f411c to c8c4d15 Compare November 8, 2019 22:00
@garimagu
Copy link
Contributor Author

garimagu commented Nov 8, 2019

Had to fix an error that appeared while merging. Please approve again.

@smaslov-intel
Copy link
Contributor

Had to fix an error that appeared while merging. Please approve again.

how can I see the change that was made since I last approved it?

@garimagu
Copy link
Contributor Author

garimagu commented Nov 8, 2019

Had to fix an error that appeared while merging. Please approve again.

how can I see the change that was made since I last approved it?

I did not create a new commit. I used commit --amend, so you cannot see that specific change.
I will make a new commit next time. This is a problem with github review system.
The change was done in : regression/memcpy-update-leafs.cpp
The update in the file in commit 5c8e81f by Alexey Voronov , changed DstOffset Variable to SrcOffset variable.
As below:
162 -// CHECK: PI ---> RT::piEnqueueMemBufferWrite( Queue, DstMem, CL_FALSE, DstOffset[0], DstAccessRange[0], SrcMem + DstOffset[0], DepEvents.size(), &DepEvents[0], &OutEvent)
163 -// CHECK-NOT: PI ---> RT::piEnqueueMemBufferWrite( Queue, DstMem, CL_FALSE, DstOffset[0], DstAccessRange[0], SrcMem + DstOffset[0], DepEvents.size(), &DepEvents[0], &OutEvent)
164 +// CHECK: PI ---> RT::piEnqueueMemBufferWrite( Queue, DstMem, CL_FALSE, DstOffset[0], DstAccessRange[0], SrcMem + SrcOffset[0], DepEvents.size(), &DepEvents[0], &OutEvent)
165 +// CHECK-NOT: PI ---> RT::piEnqueueMemBufferWrite( Queue, DstMem, CL_FALSE, DstOffset[0], DstAccessRange[0], SrcMem + SrcOffset[0], DepEvents.size(), &DepEvents[0], &OutEvent)

While merging, I missed this change in the variable name, and used the older DstOffset variable name because of which FileCheck was failing to match the CHECK Statement.

smaslov-intel
smaslov-intel previously approved these changes Nov 8, 2019
View reviewed changes
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

Thanks!

@garimagu
Copy link
Contributor Author

garimagu commented Nov 9, 2019

fail on windows is seen on device_event.cpp
This is a known issue.

bjoernknafla
bjoernknafla reviewed Nov 11, 2019
View reviewed changes
sycl/plugins/opencl/pi_opencl.cpp
void (*pfn_notify)(const char *errinfo, const void *private_info, size_t cb,
void *user_data1),
void *user_data, pi_context *retcontext) {
pi_result ret = PI_INVALID_OPERATION;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking: Most OpenCL API functions do not return CL_INVALID_OPERATION. While the CL API call should always provide an expected return value it might still be worth to initialize the return code variable to something that always works in OpenCL - even if not very helpful - for example CL_OUT_OF_RESOURCES?

(Applies to all changes of the create functions).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is in opposite good to return something that underlying run-times to not normally return like PI_INVALID_OPERATION, for cases when the run-time did not want (or could not) set its error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to document this in pi.h file where we have the declarations of the APIs.

@garimagu
Copy link
Contributor Author

@bjoernknafla @smaslov-intel
Please check and approve if you have no further comments.

smaslov-intel
smaslov-intel previously approved these changes Nov 12, 2019
View reviewed changes
@bader bader requested a review from alexbatashev November 13, 2019 10:16
@asavonic
Copy link
Contributor

A lot of tests failed on Windows. Do you plan to address them? If it is an infrastructure problem, you should probably re-run the tests to make sure you don't introduce a regression.

@garimagu
Copy link
Contributor Author

A lot of tests failed on Windows. Do you plan to address them? If it is an infrastructure problem, you should probably re-run the tests to make sure you don't introduce a regression.

Local testing did not show these errors. They are mostly infrastructure issues, since I can also see them on other PRs. I have restarted the tests and also contacted the QA team regarding this issue.

@garimagu
Copy link
Contributor Author

@romanovvlad @bader @smaslov-intel
Please re-approve for the testing to start.

smaslov-intel
smaslov-intel previously approved these changes Nov 13, 2019
View reviewed changes
bader
bader requested changes Nov 14, 2019
View reviewed changes
@garimagu
Copy link
Contributor Author

garimagu commented Nov 14, 2019 •
edited
Loading

Kindly, approve and merge. Testing will take another ~10 hours. If it is possible, please merge without waiting for the testing to complete.
I can address any further formatting issues in the next PRs. Let me know if there are any.

@bader bader merged commit be75e56 into intel:sycl Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjoernknafla bjoernknafla bjoernknafla left review comments

@agozillon agozillon agozillon left review comments

@AGindinson AGindinson AGindinson left review comments

@bader bader bader approved these changes

@smaslov-intel smaslov-intel smaslov-intel left review comments

@romanovvlad romanovvlad Awaiting requested review from romanovvlad

@alexbatashev alexbatashev Awaiting requested review from alexbatashev

Assignees

@smaslov-intel smaslov-intel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@garimagu @romanovvlad @asavonic @smaslov-intel @bjoernknafla @bader @alexbatashev @agozillon @AGindinson