Skip to content

cpp version mismatch diagnostics + tests #2297

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

Closed
wants to merge 5 commits into from
Closed

cpp version mismatch diagnostics + tests #2297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cc7735c
cpp version mismatch diagnostics + tests
jgstarIntel Aug 11, 2020
ef12e64
incorporating code review comments
jgstarIntel Sep 8, 2020
ce42c8e
Incorporating code review comments
jgstarIntel Sep 25, 2020
fd7f253
incorporating code review changes
jgstarIntel Oct 29, 2020
de8a7a8
incorporating code review issues
jgstarIntel Oct 29, 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
38 changes: 34 additions & 4 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

FD->getLanguageLinkage() == CXXLanguageLinkage &&
DC->getEnclosingNamespaceContext()->isTranslationUnit())
return true;
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated change.

Diag(Callee->getLocation(), diag::note_previous_decl) << Callee;
Diag(Caller->getLocation(), diag::note_called_by) << Caller;
}
Expand Down Expand Up @@ -2728,13 +2726,45 @@ static void emitKernelNameType(QualType T, ASTContext &Ctx, raw_ostream &OS,
emitWithoutAnonNamespaces(OS, T.getCanonicalType().getAsString(TypePolicy));
}

static int getCppVersion(Sema &S) {

// Get c++ version to support version mismatch diagnostics
int CppVersion = -1;
LangOptions LangOpts = S.getASTContext().getLangOpts();
assert(LangOpts.CPlusPlus);
if (LangOpts.CPlusPlus20)
CppVersion = 202002L;
else if (LangOpts.CPlusPlus17)
CppVersion = 201703L;
else if (LangOpts.CPlusPlus14)
CppVersion = 201402L;
else if (LangOpts.CPlusPlus11)
CppVersion = 201103L;
else
CppVersion = 199711L;

return CppVersion;
}

void SYCLIntegrationHeader::emit(raw_ostream &O) {
O << "// This is auto-generated SYCL integration header.\n";
O << "\n";

O << "#include <CL/sycl/detail/defines.hpp>\n";
O << "#include <CL/sycl/detail/kernel_desc.hpp>\n";
O << "\n";

// Get c++ version to support version mismatch diagnostics
int CppVersion = getCppVersion(S);

// Generate C++ version mismatch diagnostics
O << "#define STD_CPP_VERSION ";
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 "
Copy link
Contributor

Choose a reason for hiding this comment

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

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

"compilation\"\n";
O << "#endif\n";
O << "\n";

if (SpecConsts.size() > 0) {
Expand Down
36 changes: 36 additions & 0 deletions sycl/test/separate-compile/cpp_version_mismatch_test_1.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// >> ---- device compilation
// RUN: %clangxx -std=c++14 -fsycl-device-only -Xclang -fsycl-int-header=sycl_ihdr_a.h %s -c -I %sycl_include

// >> ---- host compilation: cpp version mismatch
// RUN: not %clangxx -std=c++11 -include sycl_ihdr_a.h -c %s -I %sycl_include 2>&1 | FileCheck %s

// >> ---- diagnostics correctness check
// CHECK: C++ version (std=c++11 or less) for host compilation cannot be matched with C++ version (std=c++14 or greater) for device compilation

//==----------- cpp_version_mismatch_test_1.cpp - SYCL separate compilation cpp
// version mismatch test -----------------==//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
// -----------------------------------------------------------------------------
#include <CL/sycl.hpp>

// This tests uses a simple example with kernel creation
// to help exercise integration header file generation
// and c++ version mismatch diagnostics generation
// In this case the compiler versions are different

namespace sycl = cl::sycl;

int main(int argc, char **argv) {

// Run empty kernel
sycl::queue deviceQueue;
deviceQueue.submit(
[&](sycl::handler &cgh) { cgh.single_task<class kernel_a>([=]() {}); });

return 0;
}
38 changes: 38 additions & 0 deletions sycl/test/separate-compile/cpp_version_mismatch_test_2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// >> ---- device compilation
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// RUN: %clangxx -std=c++14 -fsycl-device-only -Xclang -fsycl-int-header=sycl_ihdr_a.h %s -c -I %sycl_include

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 .

Copy link
Contributor

Choose a reason for hiding this comment

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

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";
|

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this idea a lot!

// CHECK-NEXT: #if (__cplusplus <= 201112L) && (STD_CPP_VERSION >= 201401L)
// CHECK-NEXT: #error "C++ version (std=c++11 or less) for host compilation
Copy link
Contributor

Choose a reason for hiding this comment

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

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

// cannot be matched with C++ version (std=c++14 or greater) for device
// compilation" CHECK-NEXT: #endif

//==----------- cpp_version_mismatch_test_2.cpp - SYCL separate compilation cpp
// version mismatch test -----------------==//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
// -----------------------------------------------------------------------------
#include <CL/sycl.hpp>

// This tests uses a simple example with kernel creation
// to help exercise integration header file generation
// and c++ version mismatch diagnostics generation
// In this case the compiler versions are the same

namespace sycl = cl::sycl;

int main(int argc, char **argv) {

// Run empty kernel
sycl::queue deviceQueue;
deviceQueue.submit(
[&](sycl::handler &cgh) { cgh.single_task<class kernel_a>([=]() {}); });

return 0;
}