Exercise 7 Implementation compatibility

codeplaysoftware / syclacademy

SYCL Academy, a set of learning materials for SYCL heterogeneous programming

https://sycl.tech

Creative Commons Attribution Share Alike 4.0 International

454 stars 102 forks source link

Exercise 7 Implementation compatibility #36

Open illuhad opened 3 years ago

illuhad commented 3 years ago

Currently, for exercise 7 (the USM exercise) there is both a DPC++ version as well as a solution with an additional ComputeCpp compatibility layer.

Can we merge them into one or create a SYCL 2020 final one? I think this might be less confusing for users
The solution uses get_info() to query for USM support. My understanding is that in SYCL 2020 final this should instead be done using the device aspect API. Can we move to the aspect API? hipSYCL only supports the aspect API to query USM, so we would need additional special cases for hipSYCL if we stick to get_info(). However, since explicit USM is mandatory, I'm not sure if it's even necessary to query, so maybe we could remove the check altogether.

EDIT: I can see that there's some benefit to keeping the USM query as it shows how to check USM support.

AerialMantis commented 3 years ago

Hi @illuhad, that's a good idea, for the IWOCL tutorial we're using the iwocl21 branch, on this branch we just have the original USM exercise as well as some additional ones which use USM, they're all currently using the ComputeCpp workaround (usm_wrapper) but I've put up a PR that should make this work for non-ComputeCpp impls as well as a few other changes (see https://github.com/codeplaysoftware/syclacademy/pull/37). I've also switched to using aspects rather than get_info for querying USM support in this patch as you suggested. Please let me know if any of these changes are problematic for hipSYCL.

illuhad commented 3 years ago

Thanks, with this PR all exercises compile cleanly with hipSYCL.

When running them, I noticed that some of the tests use specific device selectors which might not find a device depending on the target system and which backends are enabled in implementations.

While for exercise 5 I can see that the whole point is to show how to create a device selector as an example (even though the Intel GPUs that it selects for might not exist everywhere), I also see that exercises 6, 9, and 10 use gpu_selector_v when they could also just use default_selector_v which would always succeed, unlike gpu_selector_v.

Shouldn't we try to use default_selector_v as much as possible? This could potentially simplify deployment of SYCL implementations for the tutorial as users wouldn't have to worry about GPU drivers, CUDA etc and could just run on CPU to get started quickly for the purpose of the tutorial.

AerialMantis commented 3 years ago

That's great! Yeah that's a good point, these exercises originally came from a GPU programming tutorial so they were more GPU focused, I've put up a PR that switches to using the default_selector_v instead as you suggested - https://github.com/codeplaysoftware/syclacademy/pull/39. I also added a comment on the Intel selector to explain that users may want to create a different kind of selector for finding the device they want to use on their system.

keryell commented 3 years ago

Is this addressed by https://github.com/codeplaysoftware/syclacademy/pull/77 ?