Skip to content

[SYCL][joint matrix] add missing licence to test and add combination-based query #12489

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

Merged
merged 9 commits into from
Jan 29, 2024

Conversation

dkhaldi
Copy link
Contributor

@dkhaldi dkhaldi commented Jan 24, 2024

No description provided.

@dkhaldi dkhaldi requested a review from a team as a code owner January 24, 2024 21:41
Copy link
Contributor

@YuriPlyakhin YuriPlyakhin left a comment

Choose a reason for hiding this comment

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

Please, update also sycl/sycl/test-e2e/Matrix/XMX8/element_wise_all_ops.cpp, as after switching to common.hpp for other .cpp files and corresponding .hpp file, test in XMX8 folder will stop compiling.
Also despite it is temporary disabled, it passes for me locally, so it can be verified anyway that your changes are correct.

YuriPlyakhin
YuriPlyakhin previously approved these changes Jan 26, 2024
Copy link
Contributor

@YuriPlyakhin YuriPlyakhin left a comment

Choose a reason for hiding this comment

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

LGTM

@YuriPlyakhin YuriPlyakhin self-requested a review January 26, 2024 21:06
@YuriPlyakhin YuriPlyakhin dismissed their stale review January 26, 2024 21:07

Test requires further changes after discussion with author

Copy link
Contributor

@YuriPlyakhin YuriPlyakhin left a comment

Choose a reason for hiding this comment

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

LGTM, one nit comment.

Copy link
Contributor

@YuriPlyakhin YuriPlyakhin left a comment

Choose a reason for hiding this comment

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

LGTM

@dkhaldi
Copy link
Contributor Author

dkhaldi commented Jan 29, 2024

@intel/llvm-gatekeepers, all green, please help merge

@martygrant martygrant merged commit c873fe2 into intel:sycl Jan 29, 2024
@martygrant
Copy link
Contributor

@dkhaldi there is a post-commit failure. There are two errors:

/Users/runner/work/llvm/llvm/src/sycl/source/detail/queue_impl.hpp:747:64: error: unused parameter 'Type' [-Werror,-Wunused-parameter] void finalizeHandler(HandlerType &Handler, const CG::CGTYPE &Type,

/__w/llvm/llvm/src/sycl/source/detail/queue_impl.cpp:325:24: error: lambda capture 'Context' is not used [-Werror,-Wunused-lambda-capture] 325 | [&Context, &CheckEvent](const sycl::event &Event) {

@dkhaldi
Copy link
Contributor Author

dkhaldi commented Jan 29, 2024

@martygrant, should I address the errors in this PR or open a new one?

@martygrant
Copy link
Contributor

@dkhaldi apologies I should have detailed that. Yes please create a new PR for the fix, and leave a comment here to the new PR, thank you!

@dkhaldi
Copy link
Contributor Author

dkhaldi commented Jan 29, 2024

@dkhaldi apologies I should have detailed that. Yes please create a new PR for the fix, and leave a comment here to the new PR, thank you!

Sounds good, thanks!
It is weird that I cannot see these errors when building locally using
python /iusers/dkhaldi/workspaces/syclos/llvm/buildbot/compile.py

@dkhaldi
Copy link
Contributor Author

dkhaldi commented Jan 29, 2024

Also, these errors should not be caused by my PR as I don't touch this file.
My PR is a simple change to self-contained tests.

@martygrant
Copy link
Contributor

@dkhaldi apologies you are correct. These fails are coming from the last PR to be merged - #11758 the same errors are in its post-commit run - https://github.com/intel/llvm/actions/runs/7697466128. I will comment on that PR to notify the author.

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.

3 participants