Skip to content

Conversation

KornevNikita
Copy link
Contributor

@KornevNikita KornevNikita commented Aug 30, 2022

According to the SYCL2020 spec the program class should be removed since it's
now deprecated and replaced by the new class kernel_bundle.
Removing of the program_impl class will be done with a separate commit
since it's not an abi-breaking change and some performance analysis should be
done in scope of that removal.
llvm-test-suite patch: intel/llvm-test-suite#1187

According to the SYCL2020 spec the program class should be removed since it's
now deprecated and replaced by the new class kernel_bundle.
Removing of the program_impl class will be done with a separate commit
since it's not an abi-breaking change and some performance analysis should be
done in scope of that removal.
@KornevNikita KornevNikita requested a review from a team as a code owner August 30, 2022 13:36
@KornevNikita KornevNikita requested a review from againull August 30, 2022 13:36
KornevNikita added a commit to KornevNikita/llvm-test-suite that referenced this pull request Aug 30, 2022
According to SYCL2020 sycl::program is deprecated and should be removed.
Original patch: intel/llvm#6666
@KornevNikita
Copy link
Contributor Author

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

@@ -1,568 +0,0 @@
//==--------- Cache.cpp --- kernel and program cache unit test -------------==//
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we don't won't to completely lose those tests for cache mechanism. I'm not suggesting to rewrite them in scope of this PR, but we should at least submit an issue about rewriting them to kernel bundles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've disabled these tests in 5056a12 instead of removing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I've disabled these tests in 5056a12 instead of removing.

Note here: I'm ok if we remove those tests for now, but we should submit an issue about returning them back in an updated form. I also think that not only Cache test should be eventually brought back, but probably a few others as well which are being removed in this PR.

I will let someone from @intel/llvm-reviewers-runtime to comment about what is the best/right approach here, because I'm not a code owner of the component

@KornevNikita
Copy link
Contributor Author

@steffenlarsen @romanovvlad could you take a look please?

@againull
Copy link
Contributor

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

1 similar comment
@KornevNikita
Copy link
Contributor Author

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

AlexeySachkov pushed a commit to intel/llvm-test-suite that referenced this pull request Sep 5, 2022
According to SYCL2020 sycl::program is deprecated and should be removed.
Related compiler change: intel/llvm#6666
@AlexeySachkov
Copy link
Contributor

The only failed test is going to be fixed in intel/llvm-test-suite#1228

@AlexeySachkov AlexeySachkov merged commit 85a6833 into intel:sycl Sep 5, 2022
@KornevNikita KornevNikita deleted the remove-program branch September 23, 2022 10:08
steffenlarsen pushed a commit that referenced this pull request Feb 2, 2023
#8121)

These unit tests were disabled when the sycl::program class was removed
in [#6666](#6666.)
I have modified some of these unit tests to use kernel_bundles instead
of sycl::program.

I was not able to modify and re-enable all the unit tests in Cache.cpp
file because some of the older test cases were using APIs like
sycl::program.build_with_source (or compile_with_source) and, as far as
I am aware of it, there is no kernel_bundle API(s) equivalent to
sycl::program.build_with_source.
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
According to SYCL2020 sycl::program is deprecated and should be removed.
Related compiler change: intel#6666
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