Skip to content

[Driver][SYCL] Make -std=c++17 the default for DPC++ #1662

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

Merged
merged 3 commits into from
May 18, 2020
Merged

[Driver][SYCL] Make -std=c++17 the default for DPC++ #1662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec66155
[Driver][SYCL] Make -std=c++17 the default for DPC++
mdtoguchi Apr 28, 2020
23aeaad
[NFC] Update locations which state C++14 for compiling apps to C++17
mdtoguchi May 9, 2020
28a2dcf
[Driver][SYCL] Adjust default setting implementation
mdtoguchi May 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion clang/lib/Driver/ToolChains/Clang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5090,6 +5090,10 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
/*Joined=*/true);
else if (IsWindowsMSVC)
ImplyVCPPCXXVer = true;
else if (IsSYCL)
// For DPC++, we default to -std=c++17 for all compilations. Use of -std
// on the command line will override.
CmdArgs.push_back("-std=c++17");

Args.AddLastArg(CmdArgs, options::OPT_ftrigraphs,
options::OPT_fno_trigraphs);
Expand Down Expand Up @@ -5658,7 +5662,11 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,

if (LanguageStandard.empty()) {
if (IsMSVC2015Compatible)
LanguageStandard = "-std=c++14";
if (IsSYCL)
// For DPC++, C++17 is the default.
LanguageStandard = "-std=c++17";
else
LanguageStandard = "-std=c++14";
else
LanguageStandard = "-std=c++11";
}
Expand Down
13 changes: 13 additions & 0 deletions clang/test/Driver/sycl-offload.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,5 +814,18 @@
// RUN: %clangxx -### -c -fsycl -xc-header %s 2>&1 | FileCheck -check-prefixes=CHECK_XC_FSYCL %s
// CHECK_XC_FSYCL: The option -x c{{.*}} must not be used in conjunction with -fsycl{{.*}}

// -std=c++17 check (check all 3 compilations)
// RUN: %clangxx -### -c -fsycl -xc++ %s 2>&1 | FileCheck -check-prefix=CHECK-STD %s
// RUN: %clang_cl -### -c -fsycl -TP %s 2>&1 | FileCheck -check-prefix=CHECK-STD %s
// CHECK-STD: clang{{.*}} "-emit-llvm-bc" {{.*}} "-std=c++17"
// CHECK-STD: clang{{.*}} "-fsyntax-only" {{.*}} "-std=c++17"
// CHECK-STD: clang{{.*}} "-emit-obj" {{.*}} "-std=c++17"

// -std=c++17 override check
// RUN: %clangxx -### -c -fsycl -std=c++14 -xc++ %s 2>&1 | FileCheck -check-prefix=CHECK-STD-OVR %s
// RUN: %clang_cl -### -c -fsycl /std:c++14 -TP %s 2>&1 | FileCheck -check-prefix=CHECK-STD-OVR %s
// CHECK-STD-OVR: clang{{.*}} "-std=c++14"
// CHECK-STD-OVR-NOT: clang{{.*}} "-std=c++17"

// TODO: SYCL specific fail - analyze and enable
// XFAIL: windows-msvc
2 changes: 1 addition & 1 deletion sycl/doc/FAQ.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ set up a proper environment. To learn more about using the DPC++ compiler,
please refer to [Users Manual](UsersManual.md). If using a special compiler
is not an option for you and/or you would like to experiment without offloading
code to non-host devices, you can exploit SYCL's host device feature. This
gives you the ability to use any C++14 compiler. You will need to link your
gives you the ability to use any C++17 compiler. You will need to link your
application with the DPC++ Runtime library and provide a path to the SYCL
headers directory. Please, refer to your compiler manual to learn about
specific build options.
Expand Down
2 changes: 1 addition & 1 deletion sycl/doc/GetStartedGuide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ class CUDASelector : public cl::sycl::device_selector {
## C++ standard

- DPC++ runtime is built as C++14 library.
- DPC++ compiler is building apps as C++14 apps by default.
- DPC++ compiler is building apps as C++17 apps by default.

## Known Issues and Limitations

Expand Down
2 changes: 1 addition & 1 deletion sycl/test/abi/layout_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
// CHECK-NEXT: FieldDecl {{.*}} MAssociatedAccesors 'vector_class<detail::ArgDesc>':'std::vector<cl::sycl::detail::ArgDesc, std::allocator<cl::sycl::detail::ArgDesc>>'
// CHECK-NEXT: FieldDecl {{.*}} MRequirements 'vector_class<detail::Requirement *>':'std::vector<cl::sycl::detail::AccessorImplHost *, std::allocator<cl::sycl::detail::AccessorImplHost *>>'
// CHECK-NEXT: FieldDecl {{.*}} MNDRDesc 'detail::NDRDescT':'cl::sycl::detail::NDRDescT'
// CHECK-NEXT: FieldDecl {{.*}} MKernelName 'cl::sycl::string_class':'std::__cxx11::basic_string<char>'
// CHECK-NEXT: FieldDecl {{.*}} MKernelName 'cl::sycl::string_class':'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand it correctly that if we compile the application with different -std= values we expect different symbols to be exported from the runtime library?
@alexbatashev, is MKernelName really a part of the ABI? If so, can't this be a problem considering that runtime library is built with -std=c++14?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bader it is, and it can be a problem if the two types mismatch.

Consider this:

// a.h
struct A {
#ifdef BAR
  int foo;
#else 
  float foo;
#endif 
};

// b.cpp
#define BAR
#include "a.h"
extern void baz(A &a);

int main() {
  A a;
  a.foo = 10;
  baz(a);
}

// c.cpp
#include "a.h"
void baz(A &a) {
  a.foo /= 2;
}

The size of two objects is the same. The offsets of data are also the same. But we have changed the type of the field. So, in these two translation units those are really different types. And that's roughly what happens with handler fields.

That being said, I doubt, there's a difference in the layout of std::__cxx11::basic_string<char> and std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>. Neither cppreference is showing any changes in basic_string declaration. It really looks like some poor behaviour of clang: https://godbolt.org/z/XbTBaD. The demangled name with both C++14 and C++17 is the same.

Copy link
Contributor

@bader bader May 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That being said, I doubt, there's a difference in the layout of std::__cxx11::basic_string<char> and std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>. Neither cppreference is showing any changes in basic_string declaration. It really looks like some poor behaviour of clang: https://godbolt.org/z/XbTBaD. The demangled name with both C++14 and C++17 is the same.

We need to investigate what is going on here.
Not sure if this is related to the issue, IIRC, we have some code enabled only if -std=c++17 is set (e.g. CTAD rules).

Copy link
Contributor

@premanandrao premanandrao May 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The difference is because of this in <bits/basic_string.tcc>

 // Inhibit implicit instantiations for required instantiations,
 // which are defined via explicit instantiations elsewhere.
#if _GLIBCXX_EXTERN_TEMPLATE
  // The explicit instantiations definitions in src/c++11/string-inst.cc
  // are compiled as C++14, so the new C++17 members aren't instantiated.
  // Until those definitions are compiled as C++17 suppress the declaration,
  // so C++17 code will implicitly instantiate std::string and std::wstring
  // as needed.
# if __cplusplus <= 201402L && _GLIBCXX_EXTERN_TEMPLATE > 0
  extern template class basic_string<char>;
# elif ! _GLIBCXX_USE_CXX11_ABI
  // Still need to prevent implicit instantiation of the COW empty rep,
  // to ensure the definition in libstdc++.so is unique (PR 86138).
  extern template basic_string<char>::size_type
    basic_string<char>::_Rep::_S_empty_rep_storage[];
# endif
#endif // _GLIBCXX_EXTERN_TEMPLATE

With -std=c++17, we don't take the #if part of the macro (we skip the elif as well).

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 you should increment dev version of RT library along with this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is the revision set?

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 clear to me why we need to do it. Are we changing library with this change?
FWIW versioning is at #1604.

// CHECK-NEXT: FieldDecl {{.*}} MKernel 'shared_ptr_class<detail::kernel_impl>':'std::shared_ptr<cl::sycl::detail::kernel_impl>'
// CHECK-NEXT: FieldDecl {{.*}} MCGType 'detail::CG::CGTYPE':'cl::sycl::detail::CG::CGTYPE'
// CHECK-NEXT: DeclRefExpr {{.*}} 'cl::sycl::detail::CG::CGTYPE' EnumConstant {{.*}} 'NONE' 'cl::sycl::detail::CG::CGTYPE'
Expand Down