Skip to content

[SYCL] Disable std::byte code by _HAS_STD_BYTE #4834

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 6 commits into from
Nov 1, 2021

Conversation

vladimirlaz
Copy link
Contributor

@vladimirlaz vladimirlaz commented Oct 28, 2021

MS VS allows to disable std::byte type using _HAS_STD_BYTE=0
(https://devblogs.microsoft.com/cppblog/c17-features-and-stl-fixes-in-vs-2017-15-3/).

This patch disables SYCL code which relies on this type in this case.

MS VS allows to disable std::byte type using __HAS_STD_BYTE=0
(https://devblogs.microsoft.com/cppblog/c17-features-and-stl-fixes-in-vs-2017-15-3/).

This patch disables SYCL code which relies on this type in this case.
@vladimirlaz vladimirlaz requested review from againull and a team as code owners October 28, 2021 08:34
romanovvlad
romanovvlad previously approved these changes Oct 28, 2021
romanovvlad
romanovvlad previously approved these changes Oct 28, 2021
// RUN: %clangxx -fsycl -fsyntax-only -Xclang -verify -D_HAS_STD_BYTE=0 %s -Xclang -verify-ignore-unexpected=note,warning
// RUN: %clangxx -fsycl -fsyntax-only -Xclang -verify %s -Xclang -verify-ignore-unexpected=note,warning
// expected-no-diagnostics
#include <CL/sycl.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I missed that the first time. I guess the problem is exposed on Windows platform only. Am I right?

Suggested change
#include <CL/sycl.hpp>
// REQUIRES: windows
#include <CL/sycl.hpp>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But running both on Linux and Windows ensures that Linux is not regressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not blocking, but this test is useless on Linux. It does nothing useful.
You might invent infinite number of other defines to tests if they are not regressed as well. What might be useful to dump all defines enabled by including sycl.hpp and check there is nothing unexpected.

@vladimirlaz vladimirlaz requested a review from bader October 29, 2021 12:49
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.

Macro definition name in the description uses two underscores instead of one. Please, fix before merging.

@romanovvlad
Copy link
Contributor

@againull Could you please review?

@romanovvlad romanovvlad changed the title [SYCL] Disable std::byte code by __HAS_STD_BYTE [SYCL] Disable std::byte code by _HAS_STD_BYTE Nov 1, 2021
@againull againull merged commit 347b114 into intel:sycl Nov 1, 2021
@vladimirlaz vladimirlaz deleted the fix_std_byte branch November 3, 2021 08:07
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.

4 participants