cpp version mismatch diagnostics + tests #2297

jgstarIntel · 2020-08-11T14:55:49Z

Summary
Adding auto-generated C++ version mismatch diagnostics to the integration header generation files to allow version mismatch detection for separate compilation flows.

Detail
An issue occurs when the C++ version used for the device compilation equals C++14 and the version used during the host compilation equals C++11. For example, when the representation of an object in one C++ version by a compiler is different from that of another C++ version for the same object.

With this fix, the two versions (device C++ version and host C++ version) will be captured in the auto-generated integration header. When the integration header is run during the host compilation, the versions will be compared and a diagnostic can be reported.

This change set specifically covers cases where: C++ version (std=C++11 or less) for host compilation cannot be matched with C++ version (std=C++14 or greater) for device compilation.

rolandschulz · 2020-08-11T16:27:32Z

sycl/test/separate-compile/cpp_version_mismatch_test_2.cpp

+//===----------------------------------------------------------------------===//
+// -----------------------------------------------------------------------------
+#include <CL/sycl.hpp>
+#include <iostream>


You don't need an kernel doing something for this test, do you? An empty kernel is sufficient.

Suggested change

#include <iostream>

namespace sycl = cl::sycl;

int main() {

sycl::queue().submit([&](sycl::handler& cgh) {

cgh.parallel_for<class task>(sycl::nd_range<1>(16,16),

[=](auto item) {

});});

}

BTW: Why don't we write out an empty integration header if `-fsycl-int-header` is provided but there is no kernel in the source file? I can see this causing an issue if users set up their build system but some files don't (yet) have a kernel. Than the required include file is missing.

Fznamznon · 2020-08-12T09:47:05Z

sycl/test/separate-compile/cpp_version_mismatch_test_2.cpp

+// RUN: FileCheck -input-file=sycl_ihdr_a.h %s
+// CHECK: #define STD_CPP_VERSION 
+// CHECK-NEXT: #if __cplusplus != STD_CPP_VERSION
+// CHECK-NEXT: #error "C++ version for host compilation does not match C++ version used for device compilation"
+// CHECK-NEXT: #endif


Please do integration header testing in FE tests. Such tests are placed in clang/test/CodeGenSYCL .

Capturing llvm-lit test transfer into a separate issue.

What is the problem with llvm-lit?

sycl/test/separate-compile/cpp_version_mismatch_test_1.cpp

Fznamznon · 2020-08-12T09:49:43Z

clang/lib/Sema/SemaSYCL.cpp

@@ -2728,6 +2728,23 @@ static void emitKernelNameType(QualType T, ASTContext &Ctx, raw_ostream &OS,
  emitWithoutAnonNamespaces(OS, T.getCanonicalType().getAsString(TypePolicy));
 }

+int SYCLIntegrationHeader::getCppVersion() {


Doesn't clang already have similar stuff?
@erichkeane , don't you know?

I don't think we have exactly this function, but we DO similar logic for the __cpluplus macro in the preprocessor.

clang/lib/Sema/SemaSYCL.cpp

erichkeane

While I understand the motivation here, I think that there isn't a good way about this.

The __cplusplus macro is inconsistent between compilers that otherwise would be compatible (such as, we SHOULD be OK with a slightly older version of Clang where C++2a was 201911L).

Additionally, IS there a problem if the host is a slightly newer/older version?

Finally, this is going to be a maintenance headache (the rarity of is change, every 3 years, is going to result in us forgetting to update this) and massively error prone, and doesn't do what we need it to do.

I just think this is a bad idea.

erichkeane · 2020-08-12T13:07:36Z

clang/lib/Sema/SemaSYCL.cpp

@@ -2728,6 +2728,23 @@ static void emitKernelNameType(QualType T, ASTContext &Ctx, raw_ostream &OS,
  emitWithoutAnonNamespaces(OS, T.getCanonicalType().getAsString(TypePolicy));
 }

+int SYCLIntegrationHeader::getCppVersion() {


I don't think we have exactly this function, but we DO similar logic for the __cpluplus macro in the preprocessor.

clang/include/clang/Sema/Sema.h

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-08-12T13:12:15Z

clang/lib/Sema/SemaSYCL.cpp

+  if (cpp_version != -1) {
+    O << "#define STD_CPP_VERSION ";
+    O << cpp_version << "\n";
+    O << "#if __cplusplus != STD_CPP_VERSION\n";


This is actually pretty risky when using other compilers. MSVC for example used a 1997 date until just recently.

Handling windows resolve and/or linux specifiers in a separate issue/ticket

Signed-off-by: Joey Genfi <joey.genfi@intel.com>

clang/lib/Sema/SemaSYCL.cpp

Fznamznon · 2020-09-09T09:42:46Z

sycl/test/separate-compile/cpp_version_mismatch_test_2.cpp

+// RUN: FileCheck -input-file=sycl_ihdr_a.h %s
+// CHECK: #define STD_CPP_VERSION 
+// CHECK-NEXT: #if __cplusplus != STD_CPP_VERSION
+// CHECK-NEXT: #error "C++ version for host compilation does not match C++ version used for device compilation"
+// CHECK-NEXT: #endif


What is the problem with llvm-lit?

Fznamznon

I'm not going to approve this patch unless these concerns are resolved.

romanovvlad · 2020-09-16T10:42:01Z

@JoeyGIntel Could you please resolve concern mentioned above?

Signed-off-by: Joey Genfi <joey.genfi@intel.com>

bader · 2020-10-12T08:55:24Z

@JoeyGIntel, please, fix pre-commit checks.

rbegam · 2020-10-20T22:05:05Z

@JoeyGIntel, sycl tests LGTM. Please address the review comments and fix the fails.

elizabethandrews · 2020-10-29T17:40:06Z

clang/lib/Sema/SemaSYCL.cpp

@@ -210,8 +210,7 @@ bool Sema::isKnownGoodSYCLDecl(const Decl *D) {
  if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
    const IdentifierInfo *II = FD->getIdentifier();
    const DeclContext *DC = FD->getDeclContext();
-    if (II && II->isStr("__spirv_ocl_printf") &&
-        !FD->isDefined() &&
+    if (II && II->isStr("__spirv_ocl_printf") && !FD->isDefined() &&


Unrelated change.

ClangFormat probably did this when you ran it locally. If you are using the script for clang-format, please only run it for your changes i.e. the diff.

elizabethandrews · 2020-10-29T17:40:22Z

clang/lib/Sema/SemaSYCL.cpp

@@ -2298,8 +2297,7 @@ void Sema::finalizeSYCLDelayedAnalysis(const FunctionDecl *Caller,
  bool NotDefinedNoAttr = !Callee->isDefined() && !HasAttr;

  if (NotDefinedNoAttr && !Callee->getBuiltinID()) {
-    Diag(Loc, diag::err_sycl_restrict)
-        << Sema::KernelCallUndefinedFunction;
+    Diag(Loc, diag::err_sycl_restrict) << Sema::KernelCallUndefinedFunction;


Unrelated change.

elizabethandrews · 2020-10-29T17:49:59Z

sycl/test/separate-compile/cpp_version_mismatch_test_2.cpp

@@ -0,0 +1,38 @@
+// >> ---- device compilation


Since this test is only to check the content of integration header, please move this test to clang/test/CodeGenSYCL. The test will need to be modified when you move it. You can check test integration_header.cpp (in CodeGenSYCL) to see how to use the 'fake' headers and kernel calls. License information can also be removed.

elizabethandrews · 2020-10-29T18:33:27Z

clang/lib/Sema/SemaSYCL.cpp

+  O << CppVersion << "\n";
+  O << "#if (__cplusplus <= 201112L) && (STD_CPP_VERSION >= 201401L) \n";
+  O << "#error \"C++ version (std=c++11 or less) for host compilation cannot "
+       "be matched with C++ version (std=c++14 or greater) for device "


Can we change 'cannot be matched' to 'is incompatible'?

elizabethandrews · 2020-10-29T18:41:08Z

@erichkeane can you take a look at the latest patch. It has been modified to throw the error only when host version is C++11 (or less) and device version is C++14 (or greater)

erichkeane · 2020-10-29T18:45:09Z

This doesn't solve the problem actually. The issue is that MSVC until recently (and on hosts we want to support), made this number always 1997. It still supported a large amount of C++17 features at that point.

erichkeane · 2020-10-29T18:46:06Z

sycl/test/separate-compile/cpp_version_mismatch_test_2.cpp

+
+// >> ---- code generation checks
+// RUN: FileCheck -input-file=sycl_ihdr_a.h %s
+// CHECK: #define STD_CPP_VERSION


Is there a good reason to extract this into a macro instead of just putting it right into the conditional?

You mean just call getCppVersion(S) in conditional? I don't see why that can't be done. @JoeyGIntel

Actually, we ALREADY know if STD_CPP_VERSION >= 201401L, so emitting it int-header is foolish. We should just omit this enitre set of code if that isn't the case .

AND, it simplifies the patch a ton, you don't even NEED getCppVersion anymore, since you can just do:

if (LangOpts.CPlusPlus14) {
os << "#if (__cplusplus <=201112L)\n";
os << " #error ...\n";
|

I like this idea a lot!

erichkeane · 2020-10-29T18:46:37Z

sycl/test/separate-compile/cpp_version_mismatch_test_2.cpp

+// RUN: FileCheck -input-file=sycl_ihdr_a.h %s
+// CHECK: #define STD_CPP_VERSION
+// CHECK-NEXT: #if (__cplusplus <= 201112L) && (STD_CPP_VERSION >= 201401L)
+// CHECK-NEXT: #error "C++ version (std=c++11 or less) for host compilation


The wording needs work, but this is perhaps less harmful if it is a pragma-warning (assuming this isn't included as a system header).

elizabethandrews · 2020-10-29T19:04:39Z

Hmm...How can we resolve this then? For MSVC, how do we detect if host and device C++ versions are incompatible if MSVC (and other compilers if any) doesn't maintain this consistently?

erichkeane · 2020-10-29T19:11:28Z

That is why I said initially (I think...) that there just isn't a good way to implement this. Even the current implementation is going to just NOT WORK on any pre-2019 MSVC. We could detect that and just bail (#if _MSVC && __cplusplus==1997xxxx), but I'm still unconvinced this problem is sufficiently motivated.

rolandschulz · 2020-10-29T19:15:01Z

In theory it is possible to use _MSVC_LANG to check. But that is always at least C++14. Therefore if just want to check for >=C++14 it is sufficient to check for __cplusplus > 201112L || defined(_MSC_VER). Given that MSVC doesn't support /std:c++11 (or earlier) as version flag.

elizabethandrews · 2020-10-29T20:07:46Z

Thanks for explanation. I think we need to check for >=C++14 when generating the integration header (device) and <= C++11 when using the integration header (host). For the former, @erichkeane suggested using LangOpts.CPlusPlus14. Don't know off the top of my head, how clang sets this. Might be interesting to see how MSVC is handled by clang for this LangOpt.

elizabethandrews · 2020-10-29T20:09:43Z

@JoeyGIntel What is the motivation for this patch? What issues are we seeing?

erichkeane · 2020-10-29T20:15:53Z

This actually doesn't work for the compilers from microsoft BEFORE the switch to correctly reporting __cplusplus, right? The ones I'm concerned about are when __cplusplus reported 199711L despite having sufficient C++14 or C++17 support. However, properly using the _MSC_VER value would require figuring out the first version of MSVC that we believe matches the required list of supported features.

erichkeane · 2020-10-29T20:18:20Z

The LangOpts c++ versions are all set if AT LEAST that set is supported. For example, CPlusPlus17 ALSO sets CPlusPlus14, CPlusPlus11 and CPlusPlus. So checking CPlusPlus14 is sufficient to check for AT LEAST C++14.

This functionality was added in SPIR-V 1.2 and allows using an <id> to set the execution modes SubgroupsPerWorkgroupId, LocalSizeId, and LocalSizeHintI, and others. Original commit: KhronosGroup/SPIRV-LLVM-Translator@10b0aab

fix path to markdown results in benchmarks workflow

cpp version mismatch diagnostics + tests

cc7735c

jgstarIntel requested review from elizabethandrews, Fznamznon, premanandrao and a team as code owners August 11, 2020 14:55

jgstarIntel requested a review from rbegam August 11, 2020 14:55

rolandschulz reviewed Aug 11, 2020

View reviewed changes

Fznamznon reviewed Aug 12, 2020

View reviewed changes

erichkeane reviewed Aug 12, 2020

View reviewed changes

incorporating code review comments

ef12e64

Signed-off-by: Joey Genfi <joey.genfi@intel.com>

Fznamznon reviewed Sep 9, 2020

View reviewed changes

Incorporating code review comments

ce42c8e

Signed-off-by: Joey Genfi <joey.genfi@intel.com>

jgstarIntel added 2 commits October 28, 2020 19:54

incorporating code review changes

fd7f253

incorporating code review issues

de8a7a8

elizabethandrews reviewed Oct 29, 2020

View reviewed changes

erichkeane reviewed Oct 29, 2020

View reviewed changes

github-actions bot added the Stale label Feb 18, 2022

github-actions bot closed this Mar 21, 2022

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025

Merge pull request #2297 from pbalcer/fix-markdown-results

2782fbd

fix path to markdown results in benchmarks workflow

-#include <iostream>
+namespace sycl = cl::sycl;
+int main() {
+    sycl::queue().submit([&](sycl::handler& cgh) {
+        cgh.parallel_for<class task>(sycl::nd_range<1>(16,16),
+            [=](auto item) {
+            });});
+}
+BTW: Why don't we write out an empty integration header if `-fsycl-int-header` is provided but there is no kernel in the source file? I can see this causing an issue if users set up their build system but some files don't (yet) have a kernel. Than the required include file is missing.

cpp version mismatch diagnostics + tests #2297

cpp version mismatch diagnostics + tests #2297

Uh oh!

Conversation

jgstarIntel commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

romanovvlad commented Sep 16, 2020

Uh oh!

bader commented Oct 12, 2020

Uh oh!

rbegam commented Oct 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elizabethandrews commented Oct 29, 2020

Uh oh!

erichkeane commented Oct 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elizabethandrews commented Oct 29, 2020

Uh oh!

erichkeane commented Oct 29, 2020

Uh oh!

rolandschulz commented Oct 29, 2020

Uh oh!

elizabethandrews commented Oct 29, 2020

Uh oh!

elizabethandrews commented Oct 29, 2020

Uh oh!

jgstarIntel commented Aug 11, 2020 •

edited

Loading