Skip to content

[SYCL][COMPAT] First sycl_ext_oneapi_compat implementation #9755

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

Closed
wants to merge 2 commits into from

Conversation

Alcpz
Copy link
Contributor

@Alcpz Alcpz commented Jun 6, 2023

This is an implementation of the experimental extension proposed in #9646.

The compat extension has two primary goals:

  • Improve the adoption of SYCL. The compat extension is designed to provide a familiar programming interface that resembles other popular heterogeneous programming models. By reducing the learning curve, it enables developers to leverage SYCL's power and features more easily.

  • Source-to-Source Translation Support. compat is also designed to facilitate automatic source-to-source translation from other heterogeneous programming models to SYCL and offer a more standardized and consistent programming interface. This feature can significantly streamline the migration and integration of existing codebases into the SYCL ecosystem.

The PR also includes testing for the compat extension.

As we stated in the proposal PR, we are open to any suggestions, concerns, or improvements you may have, so please, let us know if you have any.

@Alcpz Alcpz requested a review from a team as a code owner June 6, 2023 13:37
@Alcpz Alcpz requested a review from cperkinsintel June 6, 2023 13:37
@Alcpz Alcpz temporarily deployed to aws June 6, 2023 13:53 — with GitHub Actions Inactive
@Alcpz
Copy link
Contributor Author

Alcpz commented Jun 7, 2023

We think that we should refactor our tests from unittests to e2e tests, the reasoning being that we are targeting device code.

Our tests are built upon gtests, so we are wondering:

  • Can we use gtests inside e2e, or should we completely avoid external dependencies there?
  • Is there any possibility to have a different path, or a different build mechanism, to include the tests included in this PR?

@cperkinsintel
Copy link
Contributor

Extensions should have docs. ( llvm/sycl/extensions/experimental/* ) , especially one as extensive as this. Also, I'd like to better understand the exact use cases. The code here seems fine, but what with wrapping atomics, kernels, and even dimensions, I'd like to get a clear understanding of what this is and how/when a user would use it.

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Jun 7, 2023

Can we use gtests inside e2e, or should we completely avoid external dependencies there?

I think we shouldn't. Not because it's an external dependency, but because both LIT and GTest are testing frameworks and it's a bad idea to mix usage of two.

I think what you're looking for is sycl/unittest-e2e with some extra infrastructure to enable on-device testing. But I don't have any particular image in mind on how that would work.

Update:
I think we need something like this:

sycl/test-e2e:
  CMakeLists.txt
  Basic/
  ... Other current subfolders ...
  gtest/ # Or maybe call it "unittest/"
  lit.cfg.py  
  ...

But also ignore the entire gtest/ from the LIT infrastructure. Then the CMake should provide an additional check-sycl-e2e-gtest target. The existing check-sycl-e2e can depend on it so that existing CI would automatically include those tests. Or maybe we should move the current part of check-sycl-e2e into check-sycl-e2e-lit and make check-sycl-e2e dependent on both *-lit and *-gtest targets.

Co-Authored-By: Gordon Brown <[email protected]>
Co-Authored-By: Joe Todd <[email protected]>
Co-Authored-By: Pietro Ghiglio <[email protected]>
Co-Authored-By: Ruyman Reyes <[email protected]>
@Alcpz Alcpz force-pushed the Alcpz/impl-sycl_ext_oneapi_compat branch from 880ebc8 to ea344df Compare June 8, 2023 15:18
@Alcpz
Copy link
Contributor Author

Alcpz commented Jun 8, 2023

Extensions should have docs. ( llvm/sycl/extensions/experimental/* )

@cperkinsintel The docs are available in #9646. The extension contains quite a bit of code so we divided it in two. I think you will find there an answer for the topics you have brought up.

@Alcpz Alcpz temporarily deployed to aws June 8, 2023 16:07 — with GitHub Actions Inactive
@AerialMantis AerialMantis requested review from a team and removed request for a team June 13, 2023 08:55
@Alcpz
Copy link
Contributor Author

Alcpz commented Jun 19, 2023

We were advised to refactor the code and split it up in multiple PRs: #9976 is the first.

@Alcpz Alcpz closed this Jun 19, 2023
@Alcpz Alcpz deleted the Alcpz/impl-sycl_ext_oneapi_compat branch August 10, 2023 15:50
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.

4 participants