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

mdtoguchi · 2020-05-08T20:07:42Z

This forces -std=c++17 for DPC++ when no -std* option is provided on
the command line. Only enabled with -fsycl

Signed-off-by: Michael D Toguchi [email protected]

This forces -std=c++17 for DPC++ when no -std* option is provided on the command line. Only enabled with -fsycl Signed-off-by: Michael D Toguchi <[email protected]>

bader · 2020-05-09T10:03:56Z

sycl/test/abi/layout_handler.cpp

@@ -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>>'


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?

@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.

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).

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).

keryell · 2020-05-09T17:39:15Z

And what if the cl::sycl::string disappears in some future SYCL?
Would it help?

dm-vodopyanov · 2020-05-09T22:03:39Z

@mdtoguchi could you please also update documentation related to this change?
E.g.,
https://github.com/intel/llvm/blame/sycl/sycl/doc/GetStartedGuide.md#L519
https://github.com/intel/llvm/blame/sycl/sycl/doc/FAQ.md#L22
Probably, somewhere else too.

Signed-off-by: Michael D Toguchi <[email protected]>

mdtoguchi · 2020-05-09T22:46:11Z

@mdtoguchi could you please also update documentation related to this change?
E.g.,
https://github.com/intel/llvm/blame/sycl/sycl/doc/GetStartedGuide.md#L519
https://github.com/intel/llvm/blame/sycl/sycl/doc/FAQ.md#L22
Probably, somewhere else too.

Update docs above which referenced C++14, to now state C++17 for application building. If there are other locations to update docs, any pointers where to look?

AGindinson

The driver part LGTM!

bader · 2020-05-14T15:07:05Z

@intel/llvm-reviewers-runtime, ping.

clang/lib/Driver/ToolChains/Clang.cpp

s-kanaev · 2020-05-14T15:24:37Z

clang/lib/Driver/ToolChains/Clang.cpp

+    Args.AddLastArg(CmdArgs, options::OPT_ftrigraphs,
+                    options::OPT_fno_trigraphs);


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

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)

s-kanaev · 2020-05-14T15:25:21Z

clang/lib/Driver/ToolChains/Clang.cpp

+    Args.AddLastArg(CmdArgs, options::OPT_ftrigraphs,
+                    options::OPT_fno_trigraphs);


Can't find check for this option in tests.

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

s-kanaev · 2020-05-14T15:26:04Z

sycl/test/abi/layout_handler.cpp

@@ -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>>'


I believe you should increment dev version of RT library along with this change.

Where is the revision set?

It's not clear to me why we need to do it. Are we changing library with this change?
FWIW versioning is at #1604.

Modify setting implementation to be more inline with existing flow. This also allows for MSVC /std option settings to override. Signed-off-by: Michael D Toguchi <[email protected]>

s-kanaev

LGTM.
I believe @AGindinson should approve also.

* sycl: (65 commits) [SYCL] Host task implementation (intel#1471) [SYCL] Update getting dependencies documentation (intel#1699) [SYCL] Fix types and transparent functors recognition in reduction (intel#1709) [SYCL][Doc] Get started guide clean-up (intel#1697) Add --spirv-fp-contract={on|off|fast} option (intel#509) [SYCL][Doc] Fix tbb target path in Get Started Guide. (intel#1695) [SYCL] Add support for kernel name types templated using enums. (intel#1675) [Driver][SYCL] Make -std=c++17 the default for DPC++ (intel#1662) AllocaInst should store Align instead of MaybeAlign. [X86] Replace selectScalarSSELoad ComplexPattern with PatFrags to handle the 3 types of loads we currently match. Harden IR and bitcode parsers against infinite size types. Revert "[nfc] test commit" [nfc] test commit Expose IRGen API to add the default IR attributes to a function definition. The release notes for ObjCBreakBeforeNestedBlockParam was placed between the release note for IndentCaseBlocks and its example code [VectorCombine] forward walk through instructions to improve chaining of transforms [PhaseOrdering] add vector reduction tests; NFC [InstCombine] Clean up alignment handling (NFC) [ARM] Patterns for VQSHRN [VectorCombine] add reduction-like patterns; NFC ...

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

ec66155

This forces -std=c++17 for DPC++ when no -std* option is provided on the command line. Only enabled with -fsycl Signed-off-by: Michael D Toguchi <[email protected]>

mdtoguchi requested review from AGindinson and a team as code owners May 8, 2020 20:07

mdtoguchi requested a review from v-klochkov May 8, 2020 20:07

mdtoguchi assigned AGindinson May 8, 2020

bader reviewed May 9, 2020

View reviewed changes

[NFC] Update locations which state C++14 for compiling apps to C++17

23aeaad

Signed-off-by: Michael D Toguchi <[email protected]>

mdtoguchi requested review from kbobrovs and pvchupin as code owners May 9, 2020 22:44

pvchupin previously approved these changes May 11, 2020

View reviewed changes

AGindinson previously approved these changes May 12, 2020

View reviewed changes

premanandrao mentioned this pull request May 14, 2020

[SYCL] Add support for kernel name types templated using enums. #1675

Merged

s-kanaev reviewed May 14, 2020

View reviewed changes

[Driver][SYCL] Adjust default setting implementation

28a2dcf

Modify setting implementation to be more inline with existing flow. This also allows for MSVC /std option settings to override. Signed-off-by: Michael D Toguchi <[email protected]>

mdtoguchi dismissed stale reviews from AGindinson and pvchupin via 28a2dcf May 14, 2020 17:31

bader requested review from s-kanaev, AGindinson and pvchupin May 17, 2020 09:44

s-kanaev approved these changes May 18, 2020

View reviewed changes

AGindinson approved these changes May 18, 2020

View reviewed changes

pvchupin approved these changes May 18, 2020

View reviewed changes

bader merged commit 3192ee7 into intel:sycl May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mdtoguchi commented May 8, 2020

bader May 9, 2020

alexbatashev May 9, 2020

bader May 9, 2020 •

edited

Loading

premanandrao May 11, 2020 •

edited

Loading

keryell commented May 9, 2020

dm-vodopyanov commented May 9, 2020

mdtoguchi commented May 9, 2020

AGindinson left a comment

bader commented May 14, 2020

s-kanaev May 14, 2020

mdtoguchi May 14, 2020

s-kanaev May 14, 2020

mdtoguchi May 14, 2020

s-kanaev May 14, 2020

mdtoguchi May 14, 2020

pvchupin May 14, 2020

s-kanaev left a comment

		Args.AddLastArg(CmdArgs, options::OPT_ftrigraphs,
		options::OPT_fno_trigraphs);

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

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

Conversation

mdtoguchi commented May 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bader May 9, 2020 • edited Loading

Choose a reason for hiding this comment

premanandrao May 11, 2020 • edited Loading

Choose a reason for hiding this comment

keryell commented May 9, 2020

dm-vodopyanov commented May 9, 2020

mdtoguchi commented May 9, 2020

AGindinson left a comment

Choose a reason for hiding this comment

bader commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-kanaev left a comment

Choose a reason for hiding this comment

bader May 9, 2020 •

edited

Loading

premanandrao May 11, 2020 •

edited

Loading