-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL] Add property validation for kernel_bundle free functions and graph extension #15647
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
[SYCL] Add property validation for kernel_bundle free functions and graph extension #15647
Conversation
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Is there SYCL spec language that specifies that exception can/may/must be thrown for invalid properties? Not sure if there's something in the SYCL-Graph extension spec that would need updated to reflect this behavior. |
} | ||
|
||
FAIL() << "Test must exit in exception handler. Exception is not thrown."; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 808287a
@@ -109,13 +109,39 @@ TEST(KernelBuildOptions, KernelBundleBasic) { | |||
sycl::kernel_bundle KernelBundle = | |||
sycl::get_kernel_bundle<sycl::bundle_state::input>(Ctx, {Dev}, | |||
{KernelID}); | |||
try { | |||
// no supported properties now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// no supported properties now | |
// unsupported property. |
Or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 808287a
spec is not yet updated but there is a question mostly if we should make exception required officially: in any case I think it would be a general rule that doesn't require update of extension spec. |
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec is not yet updated but there is a question mostly if we should make exception required officially:
"In both cases, it would be valid for the implementation to throw an exception from the constructor if the property list contains incompatible properties. Therefore, the only difference is whether the exception is mandated."
https://github.com/KhronosGroup/SYCL-Docs/issues/643
in any case I think it would be a general rule that doesn't require update of extension spec.
Thanks for following up on that with the SYCL-WG, I don't think waiting on an outcome needs to block this PR.
I have a couple of minor review comments.
sycl/include/sycl/ext/oneapi/experimental/detail/properties/graph_properties.hpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
@uditagarwal97 kindly ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SYCL Changes LGTM.
Hello @intel/llvm-gatekeepers, this PR is ready to be merged. Thanks! |
@KseniyaTikhomirova - The test results are 3 weeks old. Would you mind pushing a merge commit first? Feel free to ping me directly once it's green. |
extra changes, follow up for #15253