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] Improve error handling for kernel invocation #1209

Merged
merged 11 commits into from
Apr 8, 2020
Merged

[SYCL] Improve error handling for kernel invocation #1209

merged 11 commits into from
Apr 8, 2020

Conversation

MochalovaAn
Copy link
Contributor

Improve error handling in enqueue_kernel_launch::handleError #935

@MochalovaAn MochalovaAn changed the title Improve error handling [SYCL]Improve error handling Feb 27, 2020
@bader bader changed the title [SYCL]Improve error handling [SYCL] Improve error handling Feb 28, 2020
@bader bader changed the title [SYCL] Improve error handling [SYCL] Improve error handling for kernel invocation Feb 28, 2020
@romanovvlad
Copy link
Contributor

Could you please explain what is wrong with current error handling and how exactly you are improving it?

@MochalovaAn
Copy link
Contributor Author

As stated in this issue (github.com//issues/935) , the current handling covers only CL_INVALID_WORK_GROUP_SIZE error code . In this PR there is processing for:

  • CL_INVALID_KERNEL_ARGS
  • CL_INVALID_WORK_ITEM_SIZE
  • CL_MISALIGNED_SUB_BUFFER_OFFSET
  • CL_IMAGE_FORMAT_NOT_SUPPORTED
  • CL_MEM_OBJECT_ALLOCATION_FAILURE

@MochalovaAn MochalovaAn force-pushed the ImproveErrorHandling branch from 7e280d6 to d9ea950 Compare March 10, 2020 09:25
@MochalovaAn MochalovaAn force-pushed the ImproveErrorHandling branch from d9ea950 to 6816f7c Compare March 11, 2020 08:00
AlexeySachkov
AlexeySachkov reviewed Mar 17, 2020
View reviewed changes
sycl/source/detail/error_handling/enqueue_kernel.cpp Outdated
for (int I = 0; I < NDRDesc.Dims; I++) {
if (NDRDesc.LocalSize[I] > MaxWISize[I])
throw sycl::nd_range_error(
"Number of work-items in a work-group exceed limit for dimension "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Number of work-items in a work-group exceed limit for dimension "
"Number of work-items in a work-group exceed limit of given device for dimension "

sycl/source/detail/error_handling/enqueue_kernel.cpp Outdated Show resolved Hide resolved
sycl/source/detail/error_handling/enqueue_kernel.cpp Outdated Show resolved Hide resolved
sycl/source/detail/error_handling/enqueue_kernel.cpp Show resolved Hide resolved
@bader
Copy link
Contributor

bader commented Mar 22, 2020

@MochalovaAn, please, resolve conflicts.

@MochalovaAn
[SYCL] Improve error handling
0d6bc09 
Improve error handling in enqueue_kernel_launch::handleError.
intel#935

Signed-off-by: amochalo <anastasiya.mochalova@intel.com>
@MochalovaAn
small fix
2a41dfa 
Signed-off-by: amochalo <anastasiya.mochalova@intel.com>
@MochalovaAn
Clang-format fix
e0324d7 
Signed-off-by: amochalo <anastasiya.mochalova@intel.com>
@MochalovaAn
Clang-format fix
6d4819f 
Signed-off-by: amochalo <anastasiya.mochalova@intel.com>
@MochalovaAn
Apply comments
fea2cf9 
Signed-off-by: amochalo <anastasiya.mochalova@intel.com>
@MochalovaAn
Apply comment
ae72a62 
Signed-off-by: amochalo <anastasiya.mochalova@intel.com>
@MochalovaAn
Some fix (rebase branch)
00cc416 
Signed-off-by: amochalo <anastasiya.mochalova@intel.com>
@MochalovaAn
Small change
aaf766a 
Signed-off-by: amochalo <anastasiya.mochalova@intel.com>
@MochalovaAn
Apply comments
98842ec 
Signed-off-by: amochalo <anastasiya.mochalova@intel.com>
@MochalovaAn MochalovaAn force-pushed the ImproveErrorHandling branch from ad50247 to 98842ec Compare March 24, 2020 13:29
AlexeySachkov
AlexeySachkov reviewed Mar 24, 2020
View reviewed changes
sycl/source/detail/error_handling/enqueue_kernel.cpp Outdated Show resolved Hide resolved
sycl/source/detail/error_handling/enqueue_kernel.cpp Outdated
@@ -177,7 +178,6 @@ bool handleInvalidWorkGroupSize(const device_impl &DeviceImpl, pi_kernel Kernel,

const plugin &Plugin = DeviceImpl.getPlugin();
RT::PiDevice Device = DeviceImpl.getHandleRef();

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this change done intentionally? If no, I suggest to revert it. The same applies to a new line above (169)

@MochalovaAn
Apply comments
adf7605 
Signed-off-by: amochalo <anastasiya.mochalova@intel.com>
@MochalovaAn
Resolve warning
1f0787e 
Signed-off-by: amochalo <anastasiya.mochalova@intel.com>
@MochalovaAn MochalovaAn requested a review from bader as a code owner April 6, 2020 16:04
@mlychkov
Copy link
Contributor

mlychkov commented Apr 7, 2020 •
edited
Loading

I would recommend to squash all commits into one manually before merge.

@bader
Copy link
Contributor

bader commented Apr 7, 2020

I would recommend to squash all commits into one manually before merge.

Why?

@mlychkov
Copy link
Contributor

mlychkov commented Apr 7, 2020

I would recommend to squash all commits into one manually before merge.

Why?

Because otherwise all commits will be squashed into one automatically with merging all commit messages into one big message with useless 'Apply remarks' lines.

@bader
Copy link
Contributor

bader commented Apr 7, 2020

I would recommend to squash all commits into one manually before merge.

Why?

Because otherwise all commits will be squashed into one automatically with merging all commit messages into one big message with useless 'Apply remarks' lines.

I use either first commit message or PR description for the squashed commit message or combining the information from the commit messages/description, so there is no need to "squash manually" if it's just to create a commit message for the squashed commit.
Use PR description if first commit message is not good enough.

@mlychkov
Copy link
Contributor

mlychkov commented Apr 7, 2020

I would recommend to squash all commits into one manually before merge.

Why?

Because otherwise all commits will be squashed into one automatically with merging all commit messages into one big message with useless 'Apply remarks' lines.

I use either first commit message or PR description for the squashed commit message or combining the information from the commit messages/description, so there is no need to "squash manually" if it's just to create a commit message for the squashed commit.
Use PR description if first commit message is not good enough.

If it is so then ok, forget my comment.
I wrote it just because one of my PRs had main commit and several 'Apply remarks' commits and after merge they were squashed into one with all commit messages.

@bader
Copy link
Contributor

bader commented Apr 7, 2020 •
edited
Loading

I would recommend to squash all commits into one manually before merge.

Why?

Because otherwise all commits will be squashed into one automatically with merging all commit messages into one big message with useless 'Apply remarks' lines.

I use either first commit message or PR description for the squashed commit message or combining the information from the commit messages/description, so there is no need to "squash manually" if it's just to create a commit message for the squashed commit.
Use PR description if first commit message is not good enough.

If it is so then ok, forget my comment.
I wrote it just because one of my PRs had main commit and several 'Apply remarks' commits and after merge they were squashed into one with all commit messages.

Might happen by mistake - to err is human.

@bader bader merged commit be42ff7 into intel:sycl Apr 8, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Apr 9, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/sche…
0032e53 
…duler_docs

* origin/sycl: (26 commits)
  [Driver][SYCL] Move include/sycl header before other system header locations (intel#1492)
  [BuildBot] Improve usability of buildbot scripts (intel#1472)
  [NFC] Add GitHub actions badges to README file (intel#1496)
  [SYCL] Improve error handling for kernel invocation (intel#1209)
  [SYCL][Driver] Fix SYCL standards' handling for '-fsycl -fsycl-device-only' invocations (intel#1371)
  [SYCL] Move type checks to later in Semantic Analysis lifecycle (intel#1465)
  [CI] Download fixed versions of Python tools (intel#1485)
  [SYCL] Fix sub_group::broadcast (intel#1482)
  [SYCL][Test] Disable spec_const_redefine.cpp on all devices but HOST (intel#1488)
  [SYCL] Only export public API (intel#1456)
  [SYCL][CUDA] Fix selected_binary argument in piextDeviceSelectBinary (intel#1475)
  [SYCL] Enable LIT testing with CUDA BE (intel#1458)
  [SYCL] Fix float to half-type conversion (intel#1395)
  [NFC] Cleanup unneded macro from builtins implementation (intel#1445)
  Enable cfg-printer LLVM lit tests only if LLVM linked statically (intel#1479)
  [SYCL][NFC] Reflect the "allowlist" renaming in the code (intel#1480)
  [SYCL][Doc] Update prerequisites in GetStartedGuide (intel#1466)
  [SYCL][USM] Remove vestigial dead code (intel#1474)
  [SYCL-PTX] Fix __spirv_GroupAsyncCopy stride computation (intel#1451)
  [Driver][SYCL] Emit an error if c compilation is forced (intel#1438)
  ...
@MochalovaAn MochalovaAn deleted the ImproveErrorHandling branch April 9, 2020 14:08
@AlexeySachkov AlexeySachkov mentioned this pull request May 20, 2020
Closed
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

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@mlychkov mlychkov Awaiting requested review from mlychkov

@bader bader Awaiting requested review from bader

Assignees

@AlexeySachkov AlexeySachkov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MochalovaAn @romanovvlad @bader @mlychkov @AlexeySachkov