Skip to content

[SYCL] Throw correct exception when passing unbound accessor to command #8131

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.

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

Merged

Conversation

maarquitos14
Copy link
Contributor

@maarquitos14 maarquitos14 commented Jan 27, 2023

According to 4.7.6.9 of SYCL2020 spec, if a placeholder accessor
is passed to a command without being bound to a command group, an
exception should be thrown. Prior to this change, the exception
was only thrown if an unbound placeholder accessor is used, not
only passed. Also, prior to this change, if we passed one unbound
accessor and one (or more) bound accessors to a command, the
exception thrown was incorrect.

Closes #3078.

According to 4.7.6.9 of SYCL2020 spec, if a placeholder accessor
is passed to a command without being bound to a command group, an
exception should be thrown. Prior to this change, the exception
was only thrown if an unbound placeholder accessor is used, not
only passed. Also, prior to this change, if we passed one unbound
accessor and one (or more) bound accessors to a command, the
exception thrown was incorrect.
@maarquitos14
Copy link
Contributor Author

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

@maarquitos14 maarquitos14 temporarily deployed to aws January 27, 2023 15:46 — with GitHub Actions Inactive
@maarquitos14 maarquitos14 temporarily deployed to aws January 30, 2023 11:57 — with GitHub Actions Inactive
@maarquitos14
Copy link
Contributor Author

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

@maarquitos14 maarquitos14 temporarily deployed to aws January 30, 2023 13:13 — with GitHub Actions Inactive
@maarquitos14
Copy link
Contributor Author

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

@maarquitos14 maarquitos14 temporarily deployed to aws January 31, 2023 12:20 — with GitHub Actions Inactive
@maarquitos14 maarquitos14 temporarily deployed to aws January 31, 2023 13:06 — with GitHub Actions Inactive
@maarquitos14
Copy link
Contributor Author

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

@maarquitos14
Copy link
Contributor Author

Friendly ping @sergey-semenov @cperkinsintel @intel/llvm-reviewers-runtime

@maarquitos14 maarquitos14 temporarily deployed to aws February 3, 2023 18:03 — with GitHub Actions Inactive
@maarquitos14 maarquitos14 temporarily deployed to aws February 3, 2023 18:42 — with GitHub Actions Inactive
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

Why there are not test changes in this PR? There should either be a new test for a missing case this PR fixes or a change to enable previously disabled test (due to missing case)

@sergey-semenov
Copy link
Contributor

Actually, isn't handling this right before enqueue a bit late? The specification requires a synchronous exception in this case, which we won't get if the submitted command is blocked. It would probably be better to throw this exception from handler::finalize().

Signed-off-by: Maronas, Marcos <[email protected]>
@maarquitos14 maarquitos14 temporarily deployed to aws February 7, 2023 12:27 — with GitHub Actions Inactive
@maarquitos14
Copy link
Contributor Author

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

@maarquitos14 maarquitos14 temporarily deployed to aws February 7, 2023 16:50 — with GitHub Actions Inactive
Signed-off-by: Maronas, Marcos <[email protected]>
@maarquitos14 maarquitos14 temporarily deployed to aws February 8, 2023 12:34 — with GitHub Actions Inactive
@maarquitos14 maarquitos14 temporarily deployed to aws February 9, 2023 04:34 — with GitHub Actions Inactive
@maarquitos14 maarquitos14 temporarily deployed to aws February 9, 2023 10:38 — with GitHub Actions Inactive
@bader bader requested a review from AlexeySachkov February 9, 2023 19:45
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

I don't have objections from my side

@maarquitos14 maarquitos14 temporarily deployed to aws February 9, 2023 21:03 — with GitHub Actions Inactive
@maarquitos14 maarquitos14 temporarily deployed to aws February 9, 2023 23:52 — with GitHub Actions Inactive
@bader bader merged commit 22cefab into intel:sycl Feb 10, 2023
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.

[SYCL] Poor error reporting for unbound placeholder accessor
4 participants