[SYCL] Improve error reporting for kernel enqueue

[AlexeySachkov]

The idea is to handle error code returned from PI and based on input
arguments, emit meaningful exception with error explanation.
One of the side-effects of this is that this patch effectively allows to
use non-uniform work-groups if underlying OpenCL supports this
functionality.

[s-kanaev]

Is it even OK to put two different functionalities within single pull-request?

[bader]

Is it even OK to put two different functionalities within single pull-request?

No. It's not okay. It's recommended to keep pull requests simple and self-contained.

[AlexeySachkov]

Is it even OK to put two different functionalities within single pull-request?

No. It's not okay. It's recommended to keep pull requests simple and self-contained.

Could you please point me to where I introduce several new functionalities?

This patch restores support of non-uniform work-groups if underlying
runtime supports them.

This is done by removing part of existing error-checking: I don't see any value to check for the same error twice. Moreover, it will break new test if I leave the old code

[s-kanaev]

This is done by removing part of existing error-checking: I don't see any value to check for the same error twice. Moreover, it will break new test if I leave the old code

If support of non-uniform work-groups is required by specification then no support of it is a bug. This bug may obviously be reported and should have a distinct fix. Otherwise, if there must be no such support according to the specification I don't see any obvious reasons to let this support pass to source code.

[alexbatashev]

@AlexeySachkov I have a more general question. This WG size logic is valid for OpenCL. But PI does not require you to have an OpenCL backend. And these checks may become invalid. Should we delegate such logic to plugins, so that every plugin implements its very own rules?

[AlexeySachkov]

@AlexeySachkov I have a more general question. This WG size logic is valid for OpenCL. But PI does not require you to have an OpenCL backend. And these checks may become invalid. Should we delegate such logic to plugins, so that every plugin implements its very own rules?

Fair enough, but at this point I don't see a clear way how this can be achieved:

• There is no special spec for PI: I was told to assume that each PI call that has OpenCL equivalent should behave in this manner. (If this is not true, please correct me)
• If I understand correctly, PI has C interface, not C++. So, verbose exceptions I generated, cannot cross the border between PI plugin and SYCL RT
• Any PI plugin routine might be called from different places and contexts and it might be not always clear which error to report or whenever to report error at all: some unusual error checking might be required. (speculating here, don't have actual examples)

Tagging @smaslov-intel here

[AlexeySachkov]

This is done by removing part of existing error-checking: I don't see any value to check for the same error twice. Moreover, it will break new test if I leave the old code

If support of non-uniform work-groups is required by specification (and it really is as for method parallel_for_work_item) then no support of it is a bug. This bug may obviously be reported and should have a distinct fix.

This is not true, non-uniform work-groups are prohibited by the SYCL spec for any parallel_for variant

[smaslov-intel]

This WG size logic is valid for OpenCL. But PI does not require you to have an OpenCL backend. And these checks may become invalid.

Why is the claim that some checks are invalid in other back-ends? Is it because 1) you don't know it is possible to check through L0 (fair concern), or because 2) you know it is impossible? In any case, from now on when adding new PI calls (even new type of arguments to existing PI API) we have to ask ourselves if that's going to work with major other back-ends (L0 and CUDA).

The plan is to:

1. cover all of SYCL functionality with L0/plugin. If something cannot be covered then we have to have a discussion.
2. extend DPC++ (over SYCL) with new features and feed them into L0/plugin extending them if needed (not necessarily extending in OpenCL)

[alexbatashev]

Why is the claim that some checks are invalid in other back-ends?

This patch checks OpenCL version. Another backend will have another versioning and may not support non-uniform WG size at all. An easy solution would be to check for OpenCL 2.0+ first, and emit error about non-uniform WG size in all other cases. This way we will still be unable to handle situation when user uses custom plugin. The error message may become invalid (or should I say unexpected?), but at least it will conform to the spec.

https://github.com/intel/llvm/pull/860/files#diff-70a2b4f7b47aee28e73b8411d6d7186cR59-R94

[AlexeySachkov]

@alexbatashev

This patch checks OpenCL version. Another backend will have another versioning and may not support non-uniform WG size at all.

SYCL 1.2.1 spec explicitly built on top of OpenCL, so, any PI plugin for SYCL 1.2.1 have to emulate/mimic OpenCL (probably even better to say OpenCL 1.2).

An easy solution would be to check for OpenCL 2.0+ first, and emit error about non-uniform WG size in all other cases.

I guess this is the only solution for now. Am I missing other ways?

This way we will still be unable to handle situation when user uses custom plugin.

In any case we don't have any API to detect which PI plugin is being used and which features it supports/which limitations it has. (or at least I'm unaware of such API).

The error message may become invalid (or should I say unexpected?), but at least it will conform to the spec.

https://github.com/intel/llvm/pull/860/files#diff-70a2b4f7b47aee28e73b8411d6d7186cR59-R94

BTW, this one will be good with any PI plugin, I think. But other messages explicitly mentioning OpenCL apparently might look strange if other PI plugins are used.

[alexbatashev]

I guess this is the only solution for now. Am I missing other ways?

You're probably right. Until we have a way to report errors in a user-friendly manner from PI plugins, this is the only way.

[AlexeySachkov]

Applied most of the comments from @romanovvlad

[AlexeySachkov]

Applied rest of the comments from @romanovvlad, fixed some things in test and error handling code

[AlexeySachkov]

Improved test to fail if anything unexpected has been caught. Fixed fail on accelerator devices

[AlexeySachkov]

Looks like I used incorrect runtime for accelerator device. Adjusted the test to account all known issues in underlying runtimes used in CI.

[s-kanaev]

Seems good to me

[AlexeySachkov]

Rebased to incorporate the latest PI changes

[s-kanaev]

Looks good to me.

[bader]

@AlexeySachkov, please, avoid force-pushing. Use merge/push instead. It's easier to track changes this way.