Skip to content

Conversation

steffenlarsen
Copy link
Contributor

@steffenlarsen steffenlarsen commented May 5, 2025

This commit removes the fallback assertion implementation for SYCL kernels. The expected behavior after this is that backends that do not support native asserts, as reported through the ext_oneapi_native_assert aspect, will ignore assertions in kernel code.

This commit also makes the L0 adapter report support for native assert, as it was incorrectly reporting false before.

This commit deprecates (and removes under preview flag) the fallback
assertion implementation for SYCL kernels. The expected behavior after
this is that backends that do not support native asserts, as reported
through the ext_oneapi_native_assert aspect, will ignore assertions in
kernel code.

Signed-off-by: Larsen, Steffen <[email protected]>
@npmiller
Copy link
Contributor

npmiller commented May 5, 2025

I believe these two tests also rely on the fallback asserts:

Or at least they used to, I'm not sure the current checks represent that intent, and they never had the macros defined. But you can see the comment for the cuda/hip unsupported. And fallback is really the only case that would make sense to test for discard events with assert since it adds extra commands that might add/use events. I've been meaning to dig further into them to re-enable them for CUDA and HIP, but if we're dropping fallback we might be able to just delete these eventually.

@gmlueck
Copy link
Contributor

gmlueck commented May 5, 2025

What is our plan for the assert feature once the environment variable is removed? Will assert statements simply be silently ignored if they appear in a kernel that is submitted to a device without native support? If that is the case, I think we need an update to the sycl_ext_oneapi_assert specification.

Or, will assert be treated like other device-optional features, and the implementation will throw an exception if the device does not support it? In this case, we would also need an update to sycl_ext_oneapi_assert.

@jbrodman
Copy link
Contributor

jbrodman commented May 5, 2025

I believe these two tests also rely on the fallback asserts:

Or at least they used to, I'm not sure the current checks represent that intent, and they never had the macros defined. But you can see the comment for the cuda/hip unsupported. And fallback is really the only case that would make sense to test for discard events with assert since it adds extra commands that might add/use events. I've been meaning to dig further into them to re-enable them for CUDA and HIP, but if we're dropping fallback we might be able to just delete these eventually.

We'd ultimately like to get rid of DiscardEvents too...

Signed-off-by: Larsen, Steffen <[email protected]>
@steffenlarsen steffenlarsen requested review from a team as code owners May 5, 2025 16:03
@steffenlarsen
Copy link
Contributor Author

What is our plan for the assert feature once the environment variable is removed? Will assert statements simply be silently ignored if they appear in a kernel that is submitted to a device without native support? If that is the case, I think we need an update to the sycl_ext_oneapi_assert specification.

This seems like the safest solution to me, as it means users can still run their kernels on devices where they would run before, even if we can't do the asserts in there for them. I've updated the extension.

Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
Signed-off-by: Larsen, Steffen <[email protected]>
}
return *Img;
}
if (!Img)
Copy link
Contributor

Choose a reason for hiding this comment

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

That should go before line 1531, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we could, but line 1531 seems to do a quick check of a cast of the pointer, so it is a little inconsequential if we do this before or after. I think the benefit of having it here is if there is some path where the decompression fails and changes Img to nullptr.

#define __SYCL_USE_FALLBACK_ASSERT SYCL_FALLBACK_ASSERT
#else
#define __SYCL_USE_FALLBACK_ASSERT 0
// MSVC doesn't support #warning and we cannot use other methods to report a
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a tracker to remove this warning in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! CMPLRLLVM-70220

@jbrodman
Copy link
Contributor

jbrodman commented Sep 8, 2025

Please get your reviews in so we can finally merge this.

@steffenlarsen
Copy link
Contributor Author

Ping @intel/dpcpp-doc-reviewers & @intel/dpcpp-esimd-reviewers & @intel/unified-runtime-reviewers-level-zero

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

esimd lgtm!

@steffenlarsen
Copy link
Contributor Author

@EuphoricThinking / @nrspruit - Would you mind having a look at this, pretty please?

Copy link
Contributor

@kswiecicki kswiecicki left a comment

Choose a reason for hiding this comment

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

If the only change from L0 adapter side is misreporting native assert support fix, then LGTM.

Copy link
Contributor

@intel/llvm-gatekeepers please consider merging

@aelovikov-intel aelovikov-intel merged commit b671165 into intel:sycl Sep 10, 2025
37 checks passed
AlexeySachkov added a commit to AlexeySachkov/llvm that referenced this pull request Sep 22, 2025
This PR prepares for removal (once preview breaking changes are promoted) of
the following queue APIs:
- `ext_codeplay_supports_fusion` which has been hardcoded to `false` for
  a while now. It is also marked as deprecated by this PR
- `device_has` internal API whose only use was dropped in intel#18310
AlexeySachkov added a commit that referenced this pull request Sep 23, 2025
This PR prepares for removal (once preview breaking changes are
promoted) of the following queue APIs:
- `ext_codeplay_supports_fusion` which has been hardcoded to `false` for
a while now. It is also marked as deprecated by this PR
- `device_has` internal API whose only use was dropped in #18310
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.

10 participants