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
bader 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 1 commit
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
7 changes: 7 additions & 0 deletions clang/lib/Driver/ToolChains/Clang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5078,6 +5078,13 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
options::OPT_fno_trigraphs))
if (A != Std)
A->render(Args, CmdArgs);
} else if (IsSYCL && types::isCXX(InputType)) {
// 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);
Copy link
Contributor

@s-kanaev s-kanaev May 14, 2020

Choose a reason for hiding this comment

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

Does this really refer to enabling C++17 by default?

Copy link
Contributor Author

@mdtoguchi mdtoguchi May 14, 2020

Choose a reason for hiding this comment

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

It's a common case for the areas in which -std is added, I'm adhering to that. Perhaps this can be reworked so this isn't necessary (similar to Prem's fix)

Copy link
Contributor

@s-kanaev s-kanaev May 14, 2020

Choose a reason for hiding this comment

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

Can't find check for this option in tests.

Copy link
Contributor Author

@mdtoguchi mdtoguchi May 14, 2020

Choose a reason for hiding this comment

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

I'll rework so this addition isn't needed.

} else {
// Honor -std-default.
//
Expand Down
7 changes: 7 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,12 @@
// 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"

// TODO: SYCL specific fail - analyze and enable
// XFAIL: windows-msvc
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

@bader bader May 9, 2020

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

@alexbatashev alexbatashev May 9, 2020

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

@s-kanaev s-kanaev May 14, 2020

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

@mdtoguchi mdtoguchi May 14, 2020

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

@pvchupin pvchupin May 14, 2020

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