Skip to content

[SYCL][Graph] Add common reference semantics #16788

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 6 commits into from
Feb 13, 2025
Merged

[SYCL][Graph] Add common reference semantics #16788

merged 6 commits into from
Feb 13, 2025

Conversation

fabiomestre
Copy link
Contributor

Adds missing common reference semantic functionality such as operator==, operator!= and hash functions to all sycl graph related classes.

@fabiomestre fabiomestre force-pushed the fabio/common_reference_semantics branch from d1ac215 to 8a2571d Compare January 27, 2025 14:26
@fabiomestre fabiomestre force-pushed the fabio/common_reference_semantics branch from 8a2571d to e786630 Compare January 27, 2025 14:27
@fabiomestre fabiomestre marked this pull request as ready for review January 27, 2025 14:37
@fabiomestre fabiomestre requested a review from a team as a code owner January 27, 2025 14:37
@fabiomestre fabiomestre requested a review from Bensuo January 27, 2025 14:37
@fabiomestre fabiomestre force-pushed the fabio/common_reference_semantics branch from e786630 to c547e3f Compare January 27, 2025 14:41
Bensuo
Bensuo reviewed Jan 27, 2025
View reviewed changes
EwanC
EwanC reviewed Jan 28, 2025
View reviewed changes
@EwanC EwanC changed the title Add common reference semantics to sycl graphs [SYCL][Graph] Add common reference semantics Jan 28, 2025
@fabiomestre fabiomestre requested a review from a team as a code owner January 29, 2025 14:42
@fabiomestre
Add common reference semantics to sycl graphs
ccf64e5 
Adds missing common reference semantic functionality such
as operator==, operator!= and hash functions to all
sycl graph related classes.
@fabiomestre
Update hash unit test
eb2f79b
@fabiomestre
Address review comments
8c9c0ea
@fabiomestre fabiomestre force-pushed the fabio/common_reference_semantics branch from 1db12ad to 8c9c0ea Compare January 29, 2025 15:24
@fabiomestre
Copy link
Contributor Author

@intel/llvm-reviewers-runtime Could someone have a look at the change in this PR?

Bensuo
Bensuo reviewed Jan 31, 2025
View reviewed changes
Bensuo
Bensuo approved these changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@Bensuo Bensuo left a comment

Choose a reason for hiding this comment

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

LGTM!

@fabiomestre
Copy link
Contributor Author

@intel/llvm-gatekeepers I think this can be merged. The CI failures seem unrelated to the changes in this PR.

@EwanC
Copy link
Contributor

EwanC commented Feb 13, 2025

@intel/llvm-gatekeepers Can we merge this please? The two CI fails are SYCL :: EnqueueNativeCommand/custom-command-multiple-dev-cuda.cpp in the CUDA job and ESIMD/named_barriers/loop.cpp in the PVC job, neither of which are related to this patch

@kbenzie
Copy link
Contributor

kbenzie commented Feb 13, 2025

ESIMD/named_barriers/loop.cpp is known flaky #16598

@kbenzie kbenzie merged commit 822cf9b into intel:sycl Feb 13, 2025
16 of 17 checks passed
@kbenzie
Copy link
Contributor

kbenzie commented Feb 13, 2025

This is caused a posts commit build failure https://github.com/intel/llvm/actions/runs/13310637099/job/37172065867

@fabiomestre
Copy link
Contributor Author

fabiomestre commented Feb 13, 2025 •
edited
Loading

This is caused a posts commit build failure https://github.com/intel/llvm/actions/runs/13310637099/job/37172065867

I'm hoping removing the SYCL_EXPORTS from the templates will fix it: #17005

I'm not sure how to test the post commit build though. Anyway to run it manually for a specifc PR?

@kbenzie
Copy link
Contributor

kbenzie commented Feb 13, 2025

I'm not sure how to test the post commit build though. Anyway to run it manually for a specifc PR?

Main thing is it's a Windows build with using icx for CMAKE_C_COMPILER/CMAKE_CXX_COMPILER. Can potentially replicate that using clang-cl.

@aelovikov-intel
Copy link
Contributor

I'm not sure how to test the post commit build though. Anyway to run it manually for a specifc PR?

Push new branch to origin, then manually trigger post commit on it.

EwanC added a commit to reble/llvm that referenced this pull request Feb 13, 2025
@EwanC
[SYCL][Graph] Fix Windows build fail
a9c319c 
PR intel#16788 broke the windows
post-commit CI when building with `icx`/`icpx` https://github.com/intel/llvm/actions/runs/13310637099/job/37172065867

```
D:\github\_work\llvm\llvm\build\include\sycl\ext\oneapi\experimental\graph.hpp(577,8): error: 'dllexport' attribute ignored [-Werror,-Wignored-attributes]
  577 | struct __SYCL_EXPORT
      |        ^
D:\github\_work\llvm\llvm\build\include\sycl\detail\export.hpp(23,34): note: expanded from macro '__SYCL_EXPORT'
   23 | #define __SYCL_EXPORT __declspec(dllexport)
      |                                  ^
In file included from D:\github\_work\llvm\llvm\src\sycl\unittests\Extensions\EventMode.cpp:9:
In file included from D:\github\_work\llvm\llvm\build\include\sycl\sycl.hpp:11:
In file included from D:\github\_work\llvm\llvm\build\include\sycl\detail\core.hpp:23:
In file included from D:\github\_work\llvm\llvm\build\include\sycl\queue.hpp:34:
D:\github\_work\llvm\llvm\build\include\sycl\ext\oneapi\experimental\graph.hpp(587,8): error: 'dllexport' attribute ignored [-Werror,-Wignored-attributes]
  587 | struct __SYCL_EXPORT
      |        ^
D:\github\_work\llvm\llvm\build\include\sycl\detail\export.hpp(23,34): note: expanded from macro '__SYCL_EXPORT'
   23 | #define __SYCL_EXPORT __declspec(dllexport)
      |                                  ^
2 errors generated.
```

Fix by not exporting these symbols
This was referenced Feb 13, 2025
Closed
Merged
@fabiomestre
Copy link
Contributor Author

I'm not sure how to test the post commit build though. Anyway to run it manually for a specifc PR?

Push new branch to origin, then manually trigger post commit on it.

Thanks 👍
I triggered the post-commit for the fix here: https://github.com/intel/llvm/actions/runs/13313100130

@kbenzie
Copy link
Contributor

kbenzie commented Feb 13, 2025

I'm not sure how to test the post commit build though. Anyway to run it manually for a specifc PR?

Push new branch to origin, then manually trigger post commit on it.

Thanks 👍 I triggered the post-commit for the fix here: https://github.com/intel/llvm/actions/runs/13313100130

To confirm, these are the same changes being tested as are present in #17005?

aelovikov-intel pushed a commit that referenced this pull request Feb 13, 2025
@fabiomestre
[Sycl][Graph] Remove SYCL_EXPORT from template to fix build error (#1…
11e7c4d 
…7005)

PR #16788 broke the windows
post-commit CI when building with `icx`/`icpx`
https://github.com/intel/llvm/actions/runs/13310637099/job/37172065867

```
D:\github\_work\llvm\llvm\build\include\sycl\ext\oneapi\experimental\graph.hpp(577,8): error: 'dllexport' attribute ignored [-Werror,-Wignored-attributes]
  577 | struct __SYCL_EXPORT
      |        ^
D:\github\_work\llvm\llvm\build\include\sycl\detail\export.hpp(23,34): note: expanded from macro '__SYCL_EXPORT'
   23 | #define __SYCL_EXPORT __declspec(dllexport)
      |                                  ^
In file included from D:\github\_work\llvm\llvm\src\sycl\unittests\Extensions\EventMode.cpp:9:
In file included from D:\github\_work\llvm\llvm\build\include\sycl\sycl.hpp:11:
In file included from D:\github\_work\llvm\llvm\build\include\sycl\detail\core.hpp:23:
In file included from D:\github\_work\llvm\llvm\build\include\sycl\queue.hpp:34:
D:\github\_work\llvm\llvm\build\include\sycl\ext\oneapi\experimental\graph.hpp(587,8): error: 'dllexport' attribute ignored [-Werror,-Wignored-attributes]
  587 | struct __SYCL_EXPORT
      |        ^
D:\github\_work\llvm\llvm\build\include\sycl\detail\export.hpp(23,34): note: expanded from macro '__SYCL_EXPORT'
   23 | #define __SYCL_EXPORT __declspec(dllexport)
      |                                  ^
2 errors generated.
```

Fix by not exporting these symbols
@fabiomestre
Copy link
Contributor Author

I'm not sure how to test the post commit build though. Anyway to run it manually for a specifc PR?

Push new branch to origin, then manually trigger post commit on it.

Thanks 👍 I triggered the post-commit for the fix here: https://github.com/intel/llvm/actions/runs/13313100130

To confirm, these are the same changes being tested as are present in #17005?

Yes, they are the same changes. #17005 has been merged now. Hopefully the issue will be fixed.

@EwanC EwanC mentioned this pull request Mar 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EwanC EwanC EwanC left review comments

@cperkinsintel cperkinsintel cperkinsintel approved these changes

@Bensuo Bensuo Bensuo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fabiomestre @EwanC @kbenzie @aelovikov-intel @cperkinsintel @Bensuo