From c2b0991274101632b5f79210b90617bdeae79f8d Mon Sep 17 00:00:00 2001 From: Erich Keane Date: Mon, 7 Jun 2021 07:39:20 -0700 Subject: [PATCH 1/9] [SYCL] implement a builtin to mark a sycl kernel The unique-stable-name constraint that you can't look up the name in a constant expression before instantiating the kernel is causing issues. This provides a constexpr builtin so that you can mark the kernel without having to instantiate the kernel. --- clang/docs/LanguageExtensions.rst | 16 ++++++++++++++ clang/include/clang/Basic/TokenKinds.def | 2 ++ clang/include/clang/Sema/Sema.h | 7 +++++- clang/lib/Parse/ParseExpr.cpp | 1 + clang/lib/Sema/SemaExprCXX.cpp | 7 ++++++ clang/lib/Sema/SemaSYCL.cpp | 22 +++++++++++++++++-- .../lib/Sema/SemaTemplateInstantiateDecl.cpp | 14 ------------ sycl/include/CL/sycl/detail/kernel_desc.hpp | 2 ++ 8 files changed, 54 insertions(+), 17 deletions(-) diff --git a/clang/docs/LanguageExtensions.rst b/clang/docs/LanguageExtensions.rst index 129e73345b2a2..57734d8e5c0e1 100644 --- a/clang/docs/LanguageExtensions.rst +++ b/clang/docs/LanguageExtensions.rst @@ -2438,6 +2438,22 @@ their usual pattern without any special treatment. // Computes a unique stable name for the given type. constexpr const char * __builtin_sycl_unique_stable_name( type-id ); +``__builtin_sycl_mark_kernel_name`` +--------------------------------- + +``__builtin_sycl_mark_kernel_name`` is a builtin that can be used with +``__builtin_sycl_unique_stable_name`` to make sure a kernel is properly 'marked' +as a kernel without having to instantiate a sycl_kernel function. This is useful +for cases where the library needs to do some work with KernelInfo on the kernel +type before the instantiation. + +**Syntax**: + +.. code-block:: c + + // Computes a unique stable name for the given type. + constexpr bool __builtin_sycl_mark_kernel_name( type-id ); + Multiprecision Arithmetic Builtins ---------------------------------- diff --git a/clang/include/clang/Basic/TokenKinds.def b/clang/include/clang/Basic/TokenKinds.def index eadea5c9815cb..3730bea4fb84a 100644 --- a/clang/include/clang/Basic/TokenKinds.def +++ b/clang/include/clang/Basic/TokenKinds.def @@ -710,6 +710,8 @@ KEYWORD(__builtin_bit_cast , KEYALL) KEYWORD(__builtin_available , KEYALL) KEYWORD(__builtin_sycl_unique_stable_name, KEYSYCL) +TYPE_TRAIT_1(__builtin_sycl_mark_kernel_name, SYCLMarkKernelName, KEYSYCL) + // Clang-specific keywords enabled only in testing. TESTING_KEYWORD(__unknown_anytype , KEYALL) diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 43b3349284f18..8eacce6fde9fc 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -1069,8 +1069,13 @@ class Sema final { OpaqueParser = P; } + // Marks a type as a SYCL Kernel without necessarily adding it. Additionally, + // it diagnoses if this causes any of the evaluated + // __builtin_sycl_unique_stable_name values to change. + void MarkSYCLKernel(SourceLocation NewLoc, QualType Ty); // Does the work necessary to deal with a SYCL kernel lambda. At the moment, - // this just marks the list of lambdas required to name the kernel. + // this just marks the list of lambdas required to name the kernel. It does this + // by dispatching to MarkSYCLKernel, so it also does the diagnostics. void AddSYCLKernelLambda(const FunctionDecl *FD); class DelayedDiagnostics; diff --git a/clang/lib/Parse/ParseExpr.cpp b/clang/lib/Parse/ParseExpr.cpp index d257051549d8c..cef69c8b6491b 100644 --- a/clang/lib/Parse/ParseExpr.cpp +++ b/clang/lib/Parse/ParseExpr.cpp @@ -893,6 +893,7 @@ class CastExpressionIdValidator final : public CorrectionCandidateCallback { /// [Clang] unary-type-trait: /// '__is_aggregate' /// '__trivially_copyable' +/// '__builtin_sycl_mark_kernel_name' /// /// binary-type-trait: /// [GNU] '__is_base_of' diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp index fb82ce9bd4798..427fdae7b65af 100644 --- a/clang/lib/Sema/SemaExprCXX.cpp +++ b/clang/lib/Sema/SemaExprCXX.cpp @@ -4728,6 +4728,10 @@ static bool CheckUnaryTypeTraitTypeCompleteness(Sema &S, TypeTrait UTT, return !S.RequireCompleteType( Loc, ArgTy, diag::err_incomplete_type_used_in_type_trait_expr); + + // Only the type name matters, not the completeness, so always return true. + case UTT_SYCLMarkKernelName: + return true; } } @@ -5164,6 +5168,9 @@ static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT, return !T->isIncompleteType(); case UTT_HasUniqueObjectRepresentations: return C.hasUniqueObjectRepresentations(T); + case UTT_SYCLMarkKernelName: + Self.MarkSYCLKernel(KeyLoc, T); + return true; } } diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp index 73165983df9b1..9724a138ca498 100644 --- a/clang/lib/Sema/SemaSYCL.cpp +++ b/clang/lib/Sema/SemaSYCL.cpp @@ -5222,7 +5222,7 @@ static QualType GetSYCLKernelObjectType(const FunctionDecl *KernelCaller) { return KernelParamTy; } -void Sema::AddSYCLKernelLambda(const FunctionDecl *FD) { +void Sema::MarkSYCLKernel(SourceLocation NewLoc, QualType Ty) { auto MangleCallback = [](ASTContext &Ctx, const NamedDecl *ND) -> llvm::Optional { if (const auto *RD = dyn_cast(ND)) @@ -5232,9 +5232,27 @@ void Sema::AddSYCLKernelLambda(const FunctionDecl *FD) { return 1; }; - QualType Ty = GetSYCLKernelObjectType(FD); std::unique_ptr Ctx{ItaniumMangleContext::create( Context, Context.getDiagnostics(), MangleCallback)}; llvm::raw_null_ostream Out; Ctx->mangleTypeName(Ty, Out); + + // Evaluate whether this would change any of the already evaluated + // __builtin_sycl_unique_stable_name values. + for (auto &Itr : Context.SYCLUniqueStableNameEvaluatedValues) { + const std::string &CurName = Itr.first->ComputeName(Context); + if (Itr.second != CurName) { + Diag(NewLoc, + diag::err_kernel_invalidates_sycl_unique_stable_name); + Diag(Itr.first->getLocation(), + diag::note_sycl_unique_stable_name_evaluated_here); + // Update this so future diagnostics work correctly. + Itr.second = CurName; + } + } +} + +void Sema::AddSYCLKernelLambda(const FunctionDecl *FD) { + QualType Ty = GetSYCLKernelObjectType(FD); + MarkSYCLKernel(FD->getLocation(), Ty); } diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp index fdbdfaff2aa65..58d962d2774af 100644 --- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp @@ -773,20 +773,6 @@ static void instantiateDependentSYCLKernelAttr( // instantiation of a kernel. S.AddSYCLKernelLambda(cast(New)); - // Evaluate whether this would change any of the already evaluated - // __builtin_sycl_unique_stable_name values. - for (auto &Itr : S.Context.SYCLUniqueStableNameEvaluatedValues) { - const std::string &CurName = Itr.first->ComputeName(S.Context); - if (Itr.second != CurName) { - S.Diag(New->getLocation(), - diag::err_kernel_invalidates_sycl_unique_stable_name); - S.Diag(Itr.first->getLocation(), - diag::note_sycl_unique_stable_name_evaluated_here); - // Update this so future diagnostics work correctly. - Itr.second = CurName; - } - } - New->addAttr(Attr.clone(S.getASTContext())); } diff --git a/sycl/include/CL/sycl/detail/kernel_desc.hpp b/sycl/include/CL/sycl/detail/kernel_desc.hpp index 046c494064a36..ca4b0fb00086f 100644 --- a/sycl/include/CL/sycl/detail/kernel_desc.hpp +++ b/sycl/include/CL/sycl/detail/kernel_desc.hpp @@ -105,6 +105,8 @@ using make_index_sequence = template struct KernelInfoImpl { private: + // TODO: Do we need to 'use' this to make sure it gets instantiated? + static constexpr bool b = __builtin_sycl_mark_kernel_name(T); static constexpr auto n = __builtin_sycl_unique_stable_name(T); template static KernelInfoData impl(index_sequence) { From ddb3ae797d1d1b47db456f7c6a5d1e64249f502d Mon Sep 17 00:00:00 2001 From: Erich Keane Date: Mon, 7 Jun 2021 07:47:25 -0700 Subject: [PATCH 2/9] Fix the clang-format I'm willing to do --- clang/include/clang/Sema/Sema.h | 4 ++-- clang/lib/Sema/SemaSYCL.cpp | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 8eacce6fde9fc..a9823d865ed29 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -1074,8 +1074,8 @@ class Sema final { // __builtin_sycl_unique_stable_name values to change. void MarkSYCLKernel(SourceLocation NewLoc, QualType Ty); // Does the work necessary to deal with a SYCL kernel lambda. At the moment, - // this just marks the list of lambdas required to name the kernel. It does this - // by dispatching to MarkSYCLKernel, so it also does the diagnostics. + // this just marks the list of lambdas required to name the kernel. It does + // this by dispatching to MarkSYCLKernel, so it also does the diagnostics. void AddSYCLKernelLambda(const FunctionDecl *FD); class DelayedDiagnostics; diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp index 9724a138ca498..8054a61c84ceb 100644 --- a/clang/lib/Sema/SemaSYCL.cpp +++ b/clang/lib/Sema/SemaSYCL.cpp @@ -5242,10 +5242,9 @@ void Sema::MarkSYCLKernel(SourceLocation NewLoc, QualType Ty) { for (auto &Itr : Context.SYCLUniqueStableNameEvaluatedValues) { const std::string &CurName = Itr.first->ComputeName(Context); if (Itr.second != CurName) { - Diag(NewLoc, - diag::err_kernel_invalidates_sycl_unique_stable_name); + Diag(NewLoc, diag::err_kernel_invalidates_sycl_unique_stable_name); Diag(Itr.first->getLocation(), - diag::note_sycl_unique_stable_name_evaluated_here); + diag::note_sycl_unique_stable_name_evaluated_here); // Update this so future diagnostics work correctly. Itr.second = CurName; } From 0962bf657df5296ddc869bcea899d1a530507834 Mon Sep 17 00:00:00 2001 From: Erich Keane Date: Mon, 7 Jun 2021 09:57:19 -0700 Subject: [PATCH 3/9] Document kernel_desc.hpp use of this builtin --- sycl/include/CL/sycl/detail/kernel_desc.hpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/sycl/include/CL/sycl/detail/kernel_desc.hpp b/sycl/include/CL/sycl/detail/kernel_desc.hpp index ca4b0fb00086f..da8da2266215d 100644 --- a/sycl/include/CL/sycl/detail/kernel_desc.hpp +++ b/sycl/include/CL/sycl/detail/kernel_desc.hpp @@ -105,9 +105,17 @@ using make_index_sequence = template struct KernelInfoImpl { private: - // TODO: Do we need to 'use' this to make sure it gets instantiated? + // This is necessary to ensure that any kernels we get info for are properly + // labeled as such before we call __builtin_sycl_unique_stable_name in a + // constant expression, otherwise subsequent calls to a sycl_kernel function + // could cause the kernel name to altered, and change the result of the + // builtin. + // Additionally, we make this a dependency of 'n' so that we can guarantee + // that this is evaluated first. The builtin always returns 'true', so the + // 'else' branch of 'n's ternary is never evaluated. static constexpr bool b = __builtin_sycl_mark_kernel_name(T); - static constexpr auto n = __builtin_sycl_unique_stable_name(T); + static constexpr auto n = b ? __builtin_sycl_unique_stable_name(T) + : __builtin_sycl_unique_stable_name(T); template static KernelInfoData impl(index_sequence) { return {}; From e6c5f146d0a98a69887f5ebec3a951a887805566 Mon Sep 17 00:00:00 2001 From: Erich Keane Date: Mon, 7 Jun 2021 11:03:45 -0700 Subject: [PATCH 4/9] Fix a lang-extensions comment, update the diagnostics, add tests --- clang/docs/LanguageExtensions.rst | 2 +- .../clang/Basic/DiagnosticSemaKinds.td | 4 +- clang/include/clang/Sema/Sema.h | 2 +- clang/lib/Sema/SemaExprCXX.cpp | 2 +- clang/lib/Sema/SemaSYCL.cpp | 8 +-- clang/test/CodeGenSYCL/mark-kernel-name.cpp | 17 +++++++ clang/test/SemaSYCL/mark-kernel-name.cpp | 50 +++++++++++++++++++ 7 files changed, 77 insertions(+), 8 deletions(-) create mode 100644 clang/test/CodeGenSYCL/mark-kernel-name.cpp create mode 100644 clang/test/SemaSYCL/mark-kernel-name.cpp diff --git a/clang/docs/LanguageExtensions.rst b/clang/docs/LanguageExtensions.rst index 57734d8e5c0e1..4a7e718529450 100644 --- a/clang/docs/LanguageExtensions.rst +++ b/clang/docs/LanguageExtensions.rst @@ -2451,7 +2451,7 @@ type before the instantiation. .. code-block:: c - // Computes a unique stable name for the given type. + // Marks a type as the name of a sycl kernel. constexpr bool __builtin_sycl_mark_kernel_name( type-id ); Multiprecision Arithmetic Builtins diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 5ffbc07161f57..b32625765065a 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -6395,8 +6395,8 @@ def warn_gnu_null_ptr_arith : Warning< "arithmetic on a null pointer treated as a cast from integer to pointer is a GNU extension">, InGroup, DefaultIgnore; def err_kernel_invalidates_sycl_unique_stable_name - : Error<"kernel instantiation changes the result of an evaluated " - "'__builtin_sycl_unique_stable_name'">; + : Error<"kernel %select{naming|instantiation}0 changes the result of an " + "evaluated '__builtin_sycl_unique_stable_name'">; def note_sycl_unique_stable_name_evaluated_here : Note<"'__builtin_sycl_unique_stable_name' evaluated here">; diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index a9823d865ed29..01217c6e07153 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -1072,7 +1072,7 @@ class Sema final { // Marks a type as a SYCL Kernel without necessarily adding it. Additionally, // it diagnoses if this causes any of the evaluated // __builtin_sycl_unique_stable_name values to change. - void MarkSYCLKernel(SourceLocation NewLoc, QualType Ty); + void MarkSYCLKernel(SourceLocation NewLoc, QualType Ty, bool IsInstantiation); // Does the work necessary to deal with a SYCL kernel lambda. At the moment, // this just marks the list of lambdas required to name the kernel. It does // this by dispatching to MarkSYCLKernel, so it also does the diagnostics. diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp index 427fdae7b65af..4bf3f8f30034a 100644 --- a/clang/lib/Sema/SemaExprCXX.cpp +++ b/clang/lib/Sema/SemaExprCXX.cpp @@ -5169,7 +5169,7 @@ static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT, case UTT_HasUniqueObjectRepresentations: return C.hasUniqueObjectRepresentations(T); case UTT_SYCLMarkKernelName: - Self.MarkSYCLKernel(KeyLoc, T); + Self.MarkSYCLKernel(KeyLoc, T, /*IsInstantiation*/ false); return true; } } diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp index 8054a61c84ceb..6f6ead1c79804 100644 --- a/clang/lib/Sema/SemaSYCL.cpp +++ b/clang/lib/Sema/SemaSYCL.cpp @@ -5222,7 +5222,8 @@ static QualType GetSYCLKernelObjectType(const FunctionDecl *KernelCaller) { return KernelParamTy; } -void Sema::MarkSYCLKernel(SourceLocation NewLoc, QualType Ty) { +void Sema::MarkSYCLKernel(SourceLocation NewLoc, QualType Ty, + bool IsInstantiation) { auto MangleCallback = [](ASTContext &Ctx, const NamedDecl *ND) -> llvm::Optional { if (const auto *RD = dyn_cast(ND)) @@ -5242,7 +5243,8 @@ void Sema::MarkSYCLKernel(SourceLocation NewLoc, QualType Ty) { for (auto &Itr : Context.SYCLUniqueStableNameEvaluatedValues) { const std::string &CurName = Itr.first->ComputeName(Context); if (Itr.second != CurName) { - Diag(NewLoc, diag::err_kernel_invalidates_sycl_unique_stable_name); + Diag(NewLoc, diag::err_kernel_invalidates_sycl_unique_stable_name) + << IsInstantiation; Diag(Itr.first->getLocation(), diag::note_sycl_unique_stable_name_evaluated_here); // Update this so future diagnostics work correctly. @@ -5253,5 +5255,5 @@ void Sema::MarkSYCLKernel(SourceLocation NewLoc, QualType Ty) { void Sema::AddSYCLKernelLambda(const FunctionDecl *FD) { QualType Ty = GetSYCLKernelObjectType(FD); - MarkSYCLKernel(FD->getLocation(), Ty); + MarkSYCLKernel(FD->getLocation(), Ty, /*IsInstantiation*/ true); } diff --git a/clang/test/CodeGenSYCL/mark-kernel-name.cpp b/clang/test/CodeGenSYCL/mark-kernel-name.cpp new file mode 100644 index 0000000000000..bf5a3f69989ea --- /dev/null +++ b/clang/test/CodeGenSYCL/mark-kernel-name.cpp @@ -0,0 +1,17 @@ +// RUN: %clang_cc1 -triple x86_64-linux-pc -fsycl-is-host -disable-llvm-passes -emit-llvm %s -o - | FileCheck %s + + +int main() { + auto lambda1 = [](){}; + auto lambda2 = [](){}; + + (void)__builtin_sycl_unique_stable_name(decltype(lambda1)); + // CHECK: [17 x i8] c"_ZTSZ4mainEUlvE_\00" + + // Should change the unique-stable-name of the lambda. + (void)__builtin_sycl_mark_kernel_name(decltype(lambda2)); + (void)__builtin_sycl_unique_stable_name(decltype(lambda2)); + // CHECK: [22 x i8] c"_ZTSZ4mainEUlvE10000_\00" +} + + diff --git a/clang/test/SemaSYCL/mark-kernel-name.cpp b/clang/test/SemaSYCL/mark-kernel-name.cpp new file mode 100644 index 0000000000000..a7a7ec49b9c18 --- /dev/null +++ b/clang/test/SemaSYCL/mark-kernel-name.cpp @@ -0,0 +1,50 @@ +// RUN: %clang_cc1 %s -std=c++17 -triple x86_64-linux-gnu -Wno-sycl-2020-compat -fsycl-is-device -verify -fsyntax-only -Wno-unused + +template +[[clang::sycl_kernel]] void kernel_single_task(KernelType kernelFunc) { // #kernelSingleTask + kernelFunc(); +} + +template +struct KernelInfo { + static constexpr const char *c = __builtin_sycl_unique_stable_name(KN); // #KI_USN +}; + +template +struct FixedKernelInfo { + static constexpr bool b = __builtin_sycl_mark_kernel_name(KN); + // making 'c' dependent on 'b' is necessary to ensure 'b' gets called first. + static constexpr const char *c = b + ? __builtin_sycl_unique_stable_name(KN) + : nullptr; +}; + +template