Skip to content

[SYCL] Improve error handling for kernel invocation#1209

Merged
bader merged 11 commits intointel:syclfrom
MochalovaAn:ImproveErrorHandling
Apr 8, 2020
Merged

[SYCL] Improve error handling for kernel invocation#1209
bader merged 11 commits intointel:syclfrom
MochalovaAn:ImproveErrorHandling

Conversation

@MochalovaAn
Copy link
Contributor

@MochalovaAn MochalovaAn commented Feb 27, 2020

Improve error handling in enqueue_kernel_launch::handleError #935

@MochalovaAn MochalovaAn changed the title Improve error handling [SYCL]Improve error handling Feb 27, 2020
mlychkov
mlychkov reviewed Feb 28, 2020
View reviewed changes
@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

romanovvlad commented Mar 6, 2020

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

@MochalovaAn
Copy link
Contributor Author

MochalovaAn commented Mar 10, 2020

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
romanovvlad
romanovvlad reviewed Mar 10, 2020
View reviewed changes
@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
Copy link
Contributor

@AlexeySachkov AlexeySachkov Mar 17, 2020

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 "

@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

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

Copy link
Contributor

@AlexeySachkov AlexeySachkov Mar 24, 2020

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

@mlychkov mlychkov Awaiting requested review from mlychkov

@bader bader Awaiting requested review from bader

2 more reviewers

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@romanovvlad romanovvlad romanovvlad approved these changes

Reviewers whose approvals may not affect merge requirements

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