Skip to content

[SYCL] Disable fallback assert by default #4694

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
merged 11 commits into from
Feb 6, 2022

Conversation

s-kanaev
Copy link
Contributor

@s-kanaev s-kanaev commented Oct 4, 2021

Due excessive amount of issues submitted against fallback assert implementation the fallback assert is now disabled by default until fixed.

@s-kanaev s-kanaev requested review from bader, pvchupin and a team as code owners October 4, 2021 11:16
@s-kanaev s-kanaev requested a review from rbegam October 4, 2021 11:16
@s-kanaev s-kanaev marked this pull request as draft October 4, 2021 11:16
Sergey Kanaev added 2 commits October 6, 2021 13:44
Signed-off-by: Sergey Kanaev <[email protected]>
Signed-off-by: Sergey Kanaev <[email protected]>
@s-kanaev
Copy link
Contributor Author

s-kanaev commented Oct 6, 2021

Regressions fixed in intel/llvm-test-suite#498

gmlueck
gmlueck previously approved these changes Oct 6, 2021
@s-kanaev s-kanaev marked this pull request as ready for review October 6, 2021 13:23
@bader
Copy link
Contributor

bader commented Oct 6, 2021

@s-kanaev, please, fill the description. I miss the context - why do we need this change?

rbegam
rbegam previously approved these changes Oct 6, 2021
Copy link
Contributor

@rbegam rbegam left a comment

Choose a reason for hiding this comment

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

LGTM

@bader
Copy link
Contributor

bader commented Oct 20, 2021

@s-kanaev, please, fill the description. I miss the context - why do we need this change?

Ping.
Please, resolve merge conflicts.

@s-kanaev s-kanaev dismissed stale reviews from rbegam and gmlueck via bba24d2 October 21, 2021 13:18
@s-kanaev
Copy link
Contributor Author

Ping.
Please, resolve merge conflicts.

Done

@s-kanaev s-kanaev requested review from rbegam and gmlueck October 21, 2021 13:34
Comment on lines 70 to 71
#if defined(SYCL_ENABLE_FALLBACK_ASSERT) && !defined(__NVPTX__)
#define __SYCL_USE_FALLBACK_ASSERT 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if defined(SYCL_ENABLE_FALLBACK_ASSERT) && !defined(__NVPTX__)
#define __SYCL_USE_FALLBACK_ASSERT 1
#if defined(SYCL_ENABLE_FALLBACK_ASSERT)
#if defined(__NVPTX__)
#warning "Fallback assert is not supported on NVPTX target"
#define __SYCL_USE_FALLBACK_ASSERT 0
#else
#define __SYCL_USE_FALLBACK_ASSERT 1
#endif

Defining this macro enables the fallback assert feature even on devices without native support.

I suggest we add a note to the documentation that fallback assert is not supported on CUDA back-end.
BTW, is it supported on HIP?

@s-kanaev s-kanaev marked this pull request as ready for review January 19, 2022 12:39
@s-kanaev s-kanaev requested a review from a team as a code owner January 19, 2022 12:39
bader
bader previously approved these changes Jan 19, 2022
@s-kanaev
Copy link
Contributor Author

@pvchupin , @gmlueck , do you have any objections to the patch?

…ing of fallback assert

Signed-off-by: Sergey Kanaev <[email protected]>
@s-kanaev
Copy link
Contributor Author

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

bader
bader previously approved these changes Jan 20, 2022
Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

I think the documentation needs some tweaks.

@s-kanaev s-kanaev requested review from bader and gmlueck January 20, 2022 19:08
@s-kanaev
Copy link
Contributor Author

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

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

@s-kanaev, please, fix pre-commit fails.

@s-kanaev
Copy link
Contributor Author

s-kanaev commented Feb 4, 2022

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

@s-kanaev s-kanaev requested a review from bader February 4, 2022 21:13
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

Please, fix failures in pre-commit.

Failed Tests (9):
  SYCL :: Assert/assert_in_kernels.cpp
  SYCL :: Assert/assert_in_multiple_tus.cpp
  SYCL :: Assert/assert_in_multiple_tus_one_ndebug.cpp
  SYCL :: Assert/assert_in_one_kernel.cpp
  SYCL :: Assert/assert_in_simultaneous_kernels.cpp
  SYCL :: Assert/assert_in_simultaneously_multiple_tus.cpp
  SYCL :: Assert/assert_in_simultaneously_multiple_tus_one_ndebug.cpp
  SYCL :: DeviceLib/assert-aot.cpp
  SYCL :: DeviceLib/assert.cpp

@s-kanaev
Copy link
Contributor Author

s-kanaev commented Feb 5, 2022

Failed Tests (9):

These are resolved in intel/llvm-test-suite#754
See result in Jenkins/llvm-test-suite.

@bader bader merged commit c3e6cc2 into intel:sycl Feb 6, 2022
s-kanaev pushed a commit to s-kanaev/llvm that referenced this pull request Mar 1, 2022
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.

5 participants