Skip to content
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

[SYCL] Handler exceptions on mutually exclusive operations #4639

Merged
merged 6 commits into from
Sep 30, 2021
Merged

[SYCL] Handler exceptions on mutually exclusive operations #4639

merged 6 commits into from
Sep 30, 2021

Conversation

steffenlarsen
Copy link
Contributor

@steffenlarsen steffenlarsen commented Sep 27, 2021 •
edited
Loading

Calling handler::set_specialization_constant after or before calling handler::use_kernel_bundle should cause the latter operation to throw a SYCL exception with error code errc::invalid. These changes enforces this behavior.

This is achieved by introducing a handler_impl class that holds the current submission state. These states help detect the invalid operation order. Since adding the implementation to the handler class would be an ABI break, the handler_impl is inserted at the start of the extended members upon construction of the handler. This should be promoted in the next release that breaks ABI.

Additionally these changes moves the unit tests in sycl/unittests/SYCL2020/SpecConstDefaultValues.cpp into the more general specialization constant unit test file sycl/unittests/SYCL2020/SpecializationConstant.cpp. Three additional test cases are added to ensure the exception behavior added with this PR.

Note: This is a non-breaking ABI change.

@steffenlarsen
[SYCL] Handler exceptions on mutually exclusive operations
ef79ff6 
Calling handler::set_specialization_constant after or before calling
handler::use_kernel_bundle should cause the latter operation to throw a
SYCL exception with error code errc::invalid. These changes enforces
this behavior.

Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen requested a review from a team as a code owner September 27, 2021 13:12
@steffenlarsen
Introduce handler_impl and refactoring
4a25092 
Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>
@steffenlarsen
Fix formatting
6626bd3 
Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>
@steffenlarsen
Add new functions to Windows dump
99c4a0e 
Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>
romanovvlad
romanovvlad reviewed Sep 29, 2021
View reviewed changes
Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

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

Some comments, looking how we can move kernel_bundle to the handler_impl.

sycl/source/handler.cpp Outdated Show resolved Hide resolved
sycl/source/handler.cpp Outdated Show resolved Hide resolved
sycl/source/handler.cpp Outdated
auto HandlerImpl = getHandlerImpl(ExendedMembersVec);
if (HandlerImpl->MSubmissionState ==
detail::HandlerSubmissionState::SPEC_CONST_SET_STATE)
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we throw an exception right here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logic has been moved to handler_impl and the exceptions for setting state has been moved to there. The exception in get_specialization_constant stays in handler.hpp.

sycl/include/CL/sycl/handler.hpp Outdated Show resolved Hide resolved
sycl/source/handler.cpp Outdated Show resolved Hide resolved
@romanovvlad
Copy link
Contributor

looking how we can move kernel_bundle to the handler_impl.

I think we can do it if we introduce CG::getKernelBundle2, CG::getAdvice2 and use them in commands.cpp if original versions return nothing, but I'm not sure it's worth doing that.

@steffenlarsen
Move logic and exception to handler_impl
8391e73 
Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>
@steffenlarsen
Copy link
Contributor Author

I think we can do it if we introduce CG::getKernelBundle2, CG::getAdvice2 and use them in commands.cpp if original versions return nothing, but I'm not sure it's worth doing that.

I fear that anything built with the old headers would still expect the old extended members to be in the shared storage. Since they are shared pointers we could have them in both the extended members and in the impl, but it just further complicates the structure and frankly obfuscates the intentions.

romanovvlad
romanovvlad previously approved these changes Sep 29, 2021
View reviewed changes
sycl/source/handler.cpp Outdated Show resolved Hide resolved
@steffenlarsen
Change comments and fix windows dump
16880f2 
Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>
@romanovvlad romanovvlad merged commit 6f620a4 into intel:sycl Sep 30, 2021
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Oct 3, 2021
@alexbatashev
Merge branch 'sycl' into remove_outdated_templates
00da882 
* sycl: (108 commits)
  [SYCL][XPTI] Revisit resource management strategy (intel#4494)
  [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681)
  [SYCL] Make kernel_bundle interop more conformant (intel#4672)
  [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667)
  [SYCL] Add support for std::byte to vec class  (intel#4637)
  [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671)
  [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668)
  [SYCL] Exclude exported symbols from kernel bundles (intel#4660)
  Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659)
  [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639)
  [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657)
  [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629)
  [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663)
  [SYCL] Fix device code instrumentation (intel#4615)
  Remove myself as a code owner (intel#4653)
  [SYCL] Fix overwriting insert to sub_group_mask (intel#4656)
  [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641)
  [SYCL][Doc] Add device global extension spec (intel#4233)
  [sycl-post-link] Adds property listing exported functions (intel#4626)
  [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644)
  ...
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Oct 3, 2021
@alexbatashev
Merge branch 'sycl' into sycl_compile_perf
a4f4c4d 
* sycl: (107 commits)
  [SYCL][XPTI] Revisit resource management strategy (intel#4494)
  [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681)
  [SYCL] Make kernel_bundle interop more conformant (intel#4672)
  [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667)
  [SYCL] Add support for std::byte to vec class  (intel#4637)
  [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671)
  [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668)
  [SYCL] Exclude exported symbols from kernel bundles (intel#4660)
  Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659)
  [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639)
  [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657)
  [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629)
  [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663)
  [SYCL] Fix device code instrumentation (intel#4615)
  Remove myself as a code owner (intel#4653)
  [SYCL] Fix overwriting insert to sub_group_mask (intel#4656)
  [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641)
  [SYCL][Doc] Add device global extension spec (intel#4233)
  [sycl-post-link] Adds property listing exported functions (intel#4626)
  [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644)
  ...
@steffenlarsen steffenlarsen deleted the steffen/kernel_bundle_spec_const_exception branch December 6, 2023 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romanovvlad romanovvlad romanovvlad 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.
2 participants
@steffenlarsen @romanovvlad