Skip to content

[SYCL] Refactor C++14 support #4303

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Sep 24, 2021
Merged

[SYCL] Refactor C++14 support #4303

merged 13 commits into from
Sep 24, 2021

Conversation

romanovvlad
Copy link
Contributor

  1. Modified some constructs to avoid using C++17 features
  2. Fixed the test to better check support of different C++ versions
  3. Fixed several compiler warnings
  4. Add a warning(message) if headers are compiled in <= C++17 mode

@romanovvlad
[SYCL] Refactor C++14 support
c0fc6dd 
1. Modified some constructs to avoid using C++17 features
2. Fixed the test to better check support of different C++ versions
3. Fixed several compiler warnings
4. Add a warning(message) if headers are compiled in <= C++17 mode
@romanovvlad romanovvlad requested review from erichkeane and removed request for erichkeane August 10, 2021 13:13
erichkeane
erichkeane reviewed Aug 10, 2021
View reviewed changes
Copy link
Contributor

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

just a couple of less-helpful comments, I don't see anything in the C++ code that sticks out as incorrect.

romanovvlad
romanovvlad commented Aug 10, 2021
View reviewed changes
sycl/include/CL/sycl/detail/cg.hpp
@@ -79,8 +79,8 @@ namespace detail {
//
// 0x00000001 - CG type KERNEL version 0
// 0x01000001 - CG type KERNEL version 1
// /\
Copy link
Contributor Author

Choose a reason for hiding this comment

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

GCC complained about multi line comments.

sycl/include/CL/sycl/detail/defines_elementary.hpp Outdated
@@ -90,7 +92,7 @@
// define __SYCL_WARNING convenience macro to report compiler warnings
#if defined(__GNUC__)
#define __SYCL_GCC_PRAGMA(x) _Pragma(#x)
#define __SYCL_WARNING(msg) __SYCL_GCC_PRAGMA(GCC warning msg)
#define __SYCL_WARNING(msg) __SYCL_GCC_PRAGMA(message msg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified it to the message to align with definitions of this macro for other environments(clang, msvc). The name of the macro seems to be incorrect though.

@romanovvlad romanovvlad mentioned this pull request Aug 10, 2021
Merged
@romanovvlad
Copy link
Contributor Author

/summary:run

@romanovvlad
Copy link
Contributor Author

/summary:run

romanovvlad
romanovvlad commented Sep 20, 2021
View reviewed changes
sycl/test/basic_tests/stdcpp_compat.cpp
// RUN: %clangxx -fsycl -std=c++17 -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note,warning %s -c -o %t.out
// RUN: %clangxx -fsycl -std=c++20 -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note,warning %s -c -o %t.out
// expected-no-diagnostics
// RUN: %clangxx -std=c++14 -fsycl -Wall -pedantic -Wno-c99-extensions -Wno-deprecated -fsyntax-only -Xclang -verify=expected,cxx14 %s -c -o %t.out
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Planning to unify/merge with sycl/test/warnings/warnings.cpp in a separate PR.

@romanovvlad romanovvlad marked this pull request as ready for review September 20, 2021 15:00
@romanovvlad romanovvlad requested review from bader, pvchupin and a team as code owners September 20, 2021 15:00
romanovvlad
romanovvlad commented Sep 20, 2021
View reviewed changes
bader
bader reviewed Sep 20, 2021
View reviewed changes
v-klochkov
v-klochkov reviewed Sep 20, 2021
View reviewed changes
v-klochkov
v-klochkov previously approved these changes Sep 20, 2021
View reviewed changes
@romanovvlad
Copy link
Contributor Author

/summary:run

bader
bader requested changes Sep 23, 2021
View reviewed changes
@romanovvlad @bader
Update sycl/doc/PreprocessorMacros.md
ef92812 
Co-authored-by: Alexey Bader <alexey.bader@intel.com>
@romanovvlad romanovvlad requested a review from bader September 23, 2021 09:46
bader
bader approved these changes Sep 23, 2021
View reviewed changes
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.

sycl/doc/PreprocessorMacros.md changes lgtm

v-klochkov
v-klochkov reviewed Sep 23, 2021
View reviewed changes
sycl/include/CL/sycl/detail/cg_types.hpp
@@ -179,31 +179,29 @@ struct KernelLambdaHasKernelHandlerArgT {
// Helpers for running kernel lambda on the host device

template <typename KernelType>
typename detail::enable_if_t<
KernelLambdaHasKernelHandlerArgT<KernelType>::value, void>
typename std::enable_if_t<KernelLambdaHasKernelHandlerArgT<KernelType>::value>
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the keyword 'typename' is not required when you use std::enable_if_t, but Ok. Thank you for the additional changes anyway.

@romanovvlad romanovvlad merged commit afc90df into intel:sycl Sep 24, 2021
@pvchupin pvchupin mentioned this pull request Apr 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AaronBallman AaronBallman AaronBallman left review comments

@erichkeane erichkeane erichkeane left review comments

@bader bader bader approved these changes

@v-klochkov v-klochkov v-klochkov approved these changes

@pvchupin pvchupin Awaiting requested review from pvchupin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@romanovvlad @bader @AaronBallman @erichkeane @v-klochkov @pvchupin