[SYCL] Isolate OpenCL types and enums #6261

jchlanda · 2022-06-07T07:48:12Z

This PR populates PI enums and typedefs with values taken from the OpenCL equivalents; it also makes sure that all uses of the system OpenCL header cl.h are channelled through include/CL/sycl/detail/cl.h. Finally, pi.h and pi.hpp would only tap into the system cl.h for OpenCL backend (guarded by PI_OPENCL_AVAILABLE compiler definition), which allowed a removal of all OpenCL types and enums from other PI backends.

Fixes: #1265

kbobrovs

sycl/plugins/esimd_emulator/pi_esimd_emulator.cpp LGTM

bader

Thanks a lot for doing this clean-up!
Finally! 🥳 🎉

sycl/include/CL/sycl/detail/pi.h

sycl/plugins/cuda/pi_cuda.cpp

sycl/unittests/assert/assert.cpp

steffenlarsen

Overall I think the changes look good. I would however like if we could have some comments for the various enums specifying where the enum values come from. That should hopefully help prevent people from adding values in the future that conflict with missing and future OpenCL enum values.

sycl/include/CL/sycl/detail/pi.h

Co-authored-by: Alexey Bader <[email protected]>

sycl/include/CL/sycl/detail/pi.h

jchlanda · 2022-06-07T13:05:17Z

Overall I think the changes look good. I would however like if we could have some comments for the various enums specifying where the enum values come from. That should hopefully help prevent people from adding values in the future that conflict with missing and future OpenCL enum values.

I agree that it might be more dangerous to tinker with the enums now, would you be ok with a note in the file, something along the lines of this: https://github.com/jchlanda/llvm/blob/jakub/opencl_headers/sycl/include/CL/sycl/detail/pi.h#L59
It feels like putting a comment for every enum mapping to OpenCL (of which there are plenty) would just include a lot of noise.

smaslov-intel · 2022-06-07T13:15:00Z

Overall I think the changes look good. I would however like if we could have some comments for the various enums specifying where the enum values come from. That should hopefully help prevent people from adding values in the future that conflict with missing and future OpenCL enum values.

I agree that it might be more dangerous to tinker with the enums now, would you be ok with a note in the file, something along the lines of this: https://github.com/jchlanda/llvm/blob/jakub/opencl_headers/sycl/include/CL/sycl/detail/pi.h#L59 It feels like putting a comment for every enum mapping to OpenCL (of which there are plenty) would just include a lot of noise.

I really think we should completely untie PI enums from OpenCL, not only drop use of OpenCL names, see #6261 (comment)

jchlanda · 2022-06-07T13:27:06Z

Overall I think the changes look good. I would however like if we could have some comments for the various enums specifying where the enum values come from. That should hopefully help prevent people from adding values in the future that conflict with missing and future OpenCL enum values.

I agree that it might be more dangerous to tinker with the enums now, would you be ok with a note in the file, something along the lines of this: https://github.com/jchlanda/llvm/blob/jakub/opencl_headers/sycl/include/CL/sycl/detail/pi.h#L59 It feels like putting a comment for every enum mapping to OpenCL (of which there are plenty) would just include a lot of noise.

I really think we should completely untie PI enums from OpenCL, not only drop use of OpenCL names, see #6261 (comment)

I don't share that view, there is no hiding away from the fact that PI's interface is a direct copy of OpenCL's (albeit extended in places), coming up with a different values for effectively exactly the same enums (semantically) only to translate them back to OpenCL (even in one backend) doesn't seem like a task worth doing.

smaslov-intel · 2022-06-07T13:41:01Z

Overall I think the changes look good. I would however like if we could have some comments for the various enums specifying where the enum values come from. That should hopefully help prevent people from adding values in the future that conflict with missing and future OpenCL enum values.

I agree that it might be more dangerous to tinker with the enums now, would you be ok with a note in the file, something along the lines of this: https://github.com/jchlanda/llvm/blob/jakub/opencl_headers/sycl/include/CL/sycl/detail/pi.h#L59 It feels like putting a comment for every enum mapping to OpenCL (of which there are plenty) would just include a lot of noise.

I really think we should completely untie PI enums from OpenCL, not only drop use of OpenCL names, see #6261 (comment)

I don't share that view, there is no hiding away from the fact that PI's interface is a direct copy of OpenCL's (albeit extended in places), coming up with a different values for effectively exactly the same enums (semantically) only to translate them back to OpenCL (even in one backend) doesn't seem like a task worth doing.

This is not to "hide away" the fact that the PI is borrowing OpenCL model, but to:

Continue to be able to track the semantics of specific PI values (from seeing what OpenCL codes do they map to in OpenCL plugin)
Ensure that PI extensions aren't clashing with existing OpenCL codes.

bader

LGTM. Thanks!

jchlanda · 2022-06-08T08:37:17Z

Overall I think the changes look good. I would however like if we could have some comments for the various enums specifying where the enum values come from. That should hopefully help prevent people from adding values in the future that conflict with missing and future OpenCL enum values.

I agree that it might be more dangerous to tinker with the enums now, would you be ok with a note in the file, something along the lines of this: https://github.com/jchlanda/llvm/blob/jakub/opencl_headers/sycl/include/CL/sycl/detail/pi.h#L59 It feels like putting a comment for every enum mapping to OpenCL (of which there are plenty) would just include a lot of noise.

I really think we should completely untie PI enums from OpenCL, not only drop use of OpenCL names, see #6261 (comment)

I don't share that view, there is no hiding away from the fact that PI's interface is a direct copy of OpenCL's (albeit extended in places), coming up with a different values for effectively exactly the same enums (semantically) only to translate them back to OpenCL (even in one backend) doesn't seem like a task worth doing.

This is not to "hide away" the fact that the PI is borrowing OpenCL model, but to:
1. Continue to be able to track the semantics of specific PI values (from seeing what OpenCL codes do they map to in OpenCL plugin)

2. Ensure that PI extensions aren't clashing with existing OpenCL codes.

I understand your point now, thank you. I'm not sure if I agree though :) seems like every time OpenCL's cl.h changes it will require a review of the code in either approach, to update the mapping or to the values; given how many enums are there and the fact that some of them are used as bitfields the untying would require non-trivial amount of effort for a marginal gain.
I feel like there is value in this PR going in, so perhaps a way forward would be to create a future task, if you think it's necessary to do?

jchlanda · 2022-06-08T08:40:22Z

/verify with intel/llvm-test-suite#1048

smaslov-intel · 2022-06-08T11:42:29Z

Overall I think the changes look good. I would however like if we could have some comments for the various enums specifying where the enum values come from. That should hopefully help prevent people from adding values in the future that conflict with missing and future OpenCL enum values.

I agree that it might be more dangerous to tinker with the enums now, would you be ok with a note in the file, something along the lines of this: https://github.com/jchlanda/llvm/blob/jakub/opencl_headers/sycl/include/CL/sycl/detail/pi.h#L59 It feels like putting a comment for every enum mapping to OpenCL (of which there are plenty) would just include a lot of noise.

I really think we should completely untie PI enums from OpenCL, not only drop use of OpenCL names, see #6261 (comment)

I don't share that view, there is no hiding away from the fact that PI's interface is a direct copy of OpenCL's (albeit extended in places), coming up with a different values for effectively exactly the same enums (semantically) only to translate them back to OpenCL (even in one backend) doesn't seem like a task worth doing.

This is not to "hide away" the fact that the PI is borrowing OpenCL model, but to:
1. Continue to be able to track the semantics of specific PI values (from seeing what OpenCL codes do they map to in OpenCL plugin)

2. Ensure that PI extensions aren't clashing with existing OpenCL codes.
I understand your point now, thank you. I'm not sure if I agree though :) seems like every time OpenCL's cl.h changes it will require a review of the code in either approach, to update the mapping or to the values; given how many enums are there and the fact that some of them are used as bitfields the untying would require non-trivial amount of effort for a marginal gain. I feel like there is value in this PR going in, so perhaps a way forward would be to create a future task, if you think it's necessary to do?

I agree there is value in this PR as it is, but please retain the comment

llvm/sycl/include/CL/sycl/detail/pi.h

Line 58 in c76ef5c

// TODO: we need a mapping of PI to OpenCL somewhere, and this can be done

rephrasing it to reflect current state.

jchlanda · 2022-06-08T12:54:46Z

Done in: e948928

jchlanda · 2022-06-08T13:37:26Z

The failures should be addressed by intel/llvm-test-suite#1048, seems like the /verify with didn't do the trick.

smaslov-intel

Thanks for doing this!

bader

The failures should be addressed by intel/llvm-test-suite#1048, seems like the /verify with didn't do the trick.

It did - I see the tests pass.

bader · 2022-06-08T17:23:41Z

sycl/include/CL/sycl/detail/pi.hpp

@@ -420,6 +423,16 @@ template <class To, class From> inline To cast(From value) {
  return (To)(value);
 }

+template <class To, class From> inline To cast(std::vector<From> value) {


@jchlanda, post-commit failed with:

/home/runner/work/llvm/llvm/src/sycl/include/CL/sycl/detail/pi.hpp:426:66: error: unused parameter 'value' [-Werror,-Wunused-parameter] template <class To, class From> inline To cast(std::vector<From> value) {

Could you fix this, please?

My bad sorry, should be fixed in: #6276

No problem. Thanks for contributions!

[SYCL] Isolate OpenCL types and enums

e9061fd

jchlanda requested review from a team as code owners June 7, 2022 07:48

jchlanda requested a review from smaslov-intel June 7, 2022 07:48

Add a vector catch-all specialization to cast

e9d9df1

kbobrovs previously approved these changes Jun 7, 2022

View reviewed changes

bader previously approved these changes Jun 7, 2022

View reviewed changes

sycl/include/CL/sycl/detail/pi.h Outdated Show resolved Hide resolved

sycl/plugins/cuda/pi_cuda.cpp Outdated Show resolved Hide resolved

sycl/unittests/assert/assert.cpp Outdated Show resolved Hide resolved

steffenlarsen reviewed Jun 7, 2022

View reviewed changes

sycl/include/CL/sycl/detail/pi.h Show resolved Hide resolved

Update sycl/plugins/cuda/pi_cuda.cpp

0702e62

Co-authored-by: Alexey Bader <[email protected]>

jchlanda dismissed stale reviews from bader and kbobrovs via 0702e62 June 7, 2022 12:26

smaslov-intel reviewed Jun 7, 2022

View reviewed changes

sycl/include/CL/sycl/detail/pi.h Outdated Show resolved Hide resolved

PR comments

54ddea0

smaslov-intel reviewed Jun 7, 2022

View reviewed changes

sycl/include/CL/sycl/detail/pi.h Outdated Show resolved Hide resolved

[SYCL] Add explicit PI_ERROR to pi_result other that PI_SUCCESS

9fed447

jchlanda requested review from smaslov-intel, steffenlarsen, kbobrovs and bader June 7, 2022 13:41

bader previously approved these changes Jun 7, 2022

View reviewed changes

jchlanda mentioned this pull request Jun 8, 2022

[SYCL] Update to PI_ERROR_INVALID_OPERATION intel/llvm-test-suite#1048

Merged

PR comments

e948928

jchlanda dismissed bader’s stale review via e948928 June 8, 2022 12:53

smaslov-intel approved these changes Jun 8, 2022

View reviewed changes

jchlanda requested a review from bader June 8, 2022 15:10

bader approved these changes Jun 8, 2022

View reviewed changes

bader merged commit 3fcf848 into intel:sycl Jun 8, 2022

bader reviewed Jun 8, 2022

View reviewed changes

[SYCL] Isolate OpenCL types and enums #6261

[SYCL] Isolate OpenCL types and enums #6261

Uh oh!

Conversation

jchlanda commented Jun 7, 2022

Uh oh!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jchlanda commented Jun 7, 2022

Uh oh!

smaslov-intel commented Jun 7, 2022

Uh oh!

jchlanda commented Jun 7, 2022

Uh oh!

smaslov-intel commented Jun 7, 2022

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

jchlanda commented Jun 8, 2022

Uh oh!

jchlanda commented Jun 8, 2022

Uh oh!

smaslov-intel commented Jun 8, 2022

Uh oh!

jchlanda commented Jun 8, 2022

Uh oh!

jchlanda commented Jun 8, 2022

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

bader Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

jchlanda Jun 9, 2022

Choose a reason for hiding this comment

Uh oh!

bader Jun 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!