[SYCL] Basic diagnostics for the sycl_kernel_entry_point attribute. #120327
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `sycl_kernel_entry_point` attribute is used to declare a function that defines a pattern for an offload kernel entry point. The attribute requires a single type argument that specifies a class type that meets the requirements for a SYCL kernel name as described in section 5.2, "Naming of kernels", of the SYCL 2020 specification. A unique kernel name type is required for each function declared with the attribute. The attribute may not first appear on a declaration that follows a definition of the function. The function is required to have a `void` return type. The function must not be a non-static member function, be deleted or defaulted, be declared with the `constexpr` or `consteval` specifiers, be declared with the `[[noreturn]]` attribute, be a coroutine, or accept variadic arguments. Diagnostics are not yet provided for the following: - Use of a type as a kernel name that does not satisfy the forward declarability requirements specified in section 5.2, "Naming of kernels", of the SYCL 2020 specification. - Use of a type as a parameter of the attributed function that does not satisfy the kernel parameter requirements specified in section 4.12.4, "Rules for parameter passing to kernels", of the SYCL 2020 specification (each such function parameter constitutes a kernel parameter). - Use of language features that are not permitted in device functions as specified in section 5.4, "Language restrictions for device functions", of the SYCL 2020 specification. There are several issues noted by various FIXME comments. - The diagnostic generated for kernel name conflicts needs additional work to better detail the relevant source locations; such as the location of each declaration as well as the original source of each kernel name. - A number of the tests illustrate spurious errors being produced due to attributes that appertain to function templates being instantiated too early (during overload resolution as opposed to after an overload is selected).
tahonermann
added
clang:diagnostics
tahonermann
requested review from
bader,
AaronBallman,
jdoerfert,
erichkeane,
Endilll and
elizabethandrews
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Tom Honermann (tahonermann) ChangesThe Diagnostics are not yet provided for the following:
There are several issues noted by various FIXME comments.
Patch is 40.32 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/120327.diff 16 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 1e89a6805ce9c6..0e07c5d6ce8fba 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -3360,6 +3360,16 @@ class ASTContext : public RefCountedBase<ASTContext> {
/// this function.
void registerSYCLEntryPointFunction(FunctionDecl *FD);
+ /// Given a type used as a SYCL kernel name, returns a reference to the
+ /// metadata generated from the corresponding SYCL kernel entry point.
+ /// Aborts if the provided type is not a registered SYCL kernel name.
+ const SYCLKernelInfo &getSYCLKernelInfo(QualType T) const;
+
+ /// Returns a pointer to the metadata generated from the corresponding
+ /// SYCLkernel entry point if the provided type corresponds to a registered
+ /// SYCL kernel name. Returns a null pointer otherwise.
+ const SYCLKernelInfo *findSYCLKernelInfo(QualType T) const;
+
//===--------------------------------------------------------------------===//
// Statistics
//===--------------------------------------------------------------------===//
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 3ac490d30371b1..594e99a19b64d6 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -648,6 +648,7 @@ def PoundPragmaMessage : DiagGroup<"#pragma-messages">,
def : DiagGroup<"redundant-decls">;
def RedeclaredClassMember : DiagGroup<"redeclared-class-member">;
def GNURedeclaredEnum : DiagGroup<"gnu-redeclared-enum">;
+def RedundantAttribute : DiagGroup<"redundant-attribute">;
def RedundantMove : DiagGroup<"redundant-move">;
def Register : DiagGroup<"register", [DeprecatedRegister]>;
def ReturnTypeCLinkage : DiagGroup<"return-type-c-linkage">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 9344b620779b84..2495fbc4ce0765 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12408,6 +12408,29 @@ def err_sycl_special_type_num_init_method : Error<
"types with 'sycl_special_class' attribute must have one and only one '__init' "
"method defined">;
+// SYCL kernel entry point diagnostics
+def err_sycl_entry_point_invalid : Error<
+ "'sycl_kernel_entry_point' attribute cannot be applied to a"
+ " %select{non-static member|variadic|deleted|defaulted|constexpr|consteval|"
+ "noreturn|coroutine}0 function">;
+def err_sycl_entry_point_invalid_redeclaration : Error<
+ "'sycl_kernel_entry_point' kernel name argument does not match prior"
+ " declaration%diff{: $ vs $|}0,1">;
+def err_sycl_kernel_name_conflict : Error<
+ "'sycl_kernel_entry_point' kernel name argument conflicts with a previous"
+ " declaration">;
+def warn_sycl_kernel_name_not_a_class_type : Warning<
+ "%0 is not a valid SYCL kernel name type; a class type is required">,
+ InGroup<DiagGroup<"nonportable-sycl">>, DefaultError;
+def warn_sycl_entry_point_redundant_declaration : Warning<
+ "redundant 'sycl_kernel_entry_point' attribute">, InGroup<RedundantAttribute>;
+def err_sycl_entry_point_after_definition : Error<
+ "'sycl_kernel_entry_point' attribute cannot be added to a function after the"
+ " function is defined">;
+def err_sycl_entry_point_return_type : Error<
+ "'sycl_kernel_entry_point' attribute only applies to functions with a"
+ " 'void' return type">;
+
def warn_cuda_maxclusterrank_sm_90 : Warning<
"maxclusterrank requires sm_90 or higher, CUDA arch provided: %0, ignoring "
"%1 attribute">, InGroup<IgnoredAttributes>;
diff --git a/clang/include/clang/Sema/SemaSYCL.h b/clang/include/clang/Sema/SemaSYCL.h
index c9f3358124eda7..5bb0de40c886c7 100644
--- a/clang/include/clang/Sema/SemaSYCL.h
+++ b/clang/include/clang/Sema/SemaSYCL.h
@@ -63,6 +63,8 @@ class SemaSYCL : public SemaBase {
void handleKernelAttr(Decl *D, const ParsedAttr &AL);
void handleKernelEntryPointAttr(Decl *D, const ParsedAttr &AL);
+
+ void CheckSYCLEntryPointFunctionDecl(FunctionDecl *FD);
};
} // namespace clang
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 6ec927e13a7552..3c9614c6cee2bb 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -14463,6 +14463,19 @@ void ASTContext::registerSYCLEntryPointFunction(FunctionDecl *FD) {
std::make_pair(KernelNameType, BuildSYCLKernelInfo(KernelNameType, FD)));
}
+const SYCLKernelInfo &ASTContext::getSYCLKernelInfo(QualType T) const {
+ CanQualType KernelNameType = getCanonicalType(T);
+ return SYCLKernels.at(KernelNameType);
+}
+
+const SYCLKernelInfo *ASTContext::findSYCLKernelInfo(QualType T) const {
+ CanQualType KernelNameType = getCanonicalType(T);
+ auto IT = SYCLKernels.find(KernelNameType);
+ if (IT != SYCLKernels.end())
+ return &IT->second;
+ return nullptr;
+}
+
OMPTraitInfo &ASTContext::getNewOMPTraitInfo() {
OMPTraitInfoVector.emplace_back(new OMPTraitInfo());
return *OMPTraitInfoVector.back();
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 55e891e3acf20d..aea8bb112c0271 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -53,6 +53,7 @@
#include "clang/Sema/SemaPPC.h"
#include "clang/Sema/SemaRISCV.h"
#include "clang/Sema/SemaSwift.h"
+#include "clang/Sema/SemaSYCL.h"
#include "clang/Sema/SemaWasm.h"
#include "clang/Sema/Template.h"
#include "llvm/ADT/STLForwardCompat.h"
@@ -3018,6 +3019,15 @@ static void checkNewAttributesAfterDef(Sema &S, Decl *New, const Decl *Old) {
// declarations after definitions.
++I;
continue;
+ } else if (isa<SYCLKernelEntryPointAttr>(NewAttribute)) {
+ // Elevate latent uses of the sycl_kernel_entry_point attribute to an
+ // error since the definition will have already been created without
+ // the semantic effects of the attribute having been applied.
+ S.Diag(NewAttribute->getLocation(),
+ diag::err_sycl_entry_point_after_definition);
+ S.Diag(Def->getLocation(), diag::note_previous_definition);
+ ++I;
+ continue;
}
S.Diag(NewAttribute->getLocation(),
@@ -12146,7 +12156,7 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,
OpenMP().ActOnFinishedFunctionDefinitionInOpenMPAssumeScope(NewFD);
if (LangOpts.isSYCL() && NewFD->hasAttr<SYCLKernelEntryPointAttr>())
- getASTContext().registerSYCLEntryPointFunction(NewFD);
+ SYCL().CheckSYCLEntryPointFunctionDecl(NewFD);
// Semantic checking for this function declaration (in isolation).
@@ -15978,6 +15988,24 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
CheckCoroutineWrapper(FD);
}
+ // Diagnose invalid SYCL kernel entry point function declarations.
+ if (FD && !FD->isInvalidDecl() && !FD->isTemplated() &&
+ FD->hasAttr<SYCLKernelEntryPointAttr>()) {
+ if (FD->isDeleted()) {
+ Diag(FD->getAttr<SYCLKernelEntryPointAttr>()->getLocation(),
+ diag::err_sycl_entry_point_invalid)
+ << /*deleted function*/2;
+ } else if (FD->isDefaulted()) {
+ Diag(FD->getAttr<SYCLKernelEntryPointAttr>()->getLocation(),
+ diag::err_sycl_entry_point_invalid)
+ << /*defaulted function*/3;
+ } else if (FSI->isCoroutine()) {
+ Diag(FD->getAttr<SYCLKernelEntryPointAttr>()->getLocation(),
+ diag::err_sycl_entry_point_invalid)
+ << /*coroutine*/7;
+ }
+ }
+
{
// Do not call PopExpressionEvaluationContext() if it is a lambda because
// one is already popped when finishing the lambda in BuildLambdaExpr().
diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp
index d4fddeb01d0fc8..04ab8c28e14108 100644
--- a/clang/lib/Sema/SemaSYCL.cpp
+++ b/clang/lib/Sema/SemaSYCL.cpp
@@ -10,7 +10,9 @@
#include "clang/Sema/SemaSYCL.h"
#include "clang/AST/Mangle.h"
+#include "clang/AST/SYCLKernelInfo.h"
#include "clang/AST/TypeOrdering.h"
+#include "clang/Basic/Diagnostic.h"
#include "clang/Sema/Attr.h"
#include "clang/Sema/ParsedAttr.h"
#include "clang/Sema/Sema.h"
@@ -206,3 +208,127 @@ void SemaSYCL::handleKernelEntryPointAttr(Decl *D, const ParsedAttr &AL) {
D->addAttr(::new (SemaRef.Context)
SYCLKernelEntryPointAttr(SemaRef.Context, AL, TSI));
}
+
+static SourceLocation SourceLocationForType(QualType QT) {
+ SourceLocation Loc;
+ const Type *T = QT->getUnqualifiedDesugaredType();
+ if (const TagType *TT = dyn_cast<TagType>(T))
+ Loc = TT->getDecl()->getLocation();
+ else if (const ObjCInterfaceType *ObjCIT = dyn_cast<ObjCInterfaceType>(T))
+ Loc = ObjCIT->getDecl()->getLocation();
+ return Loc;
+}
+
+static bool CheckSYCLKernelName(Sema &S, SourceLocation Loc,
+ QualType KernelName) {
+ assert(!KernelName->isDependentType());
+
+ if (!KernelName->isStructureOrClassType()) {
+ // SYCL 2020 section 5.2, "Naming of kernels", only requires that the
+ // kernel name be a C++ typename. However, the definition of "kernel name"
+ // in the glossary states that a kernel name is a class type. Neither
+ // section explicitly states whether the kernel name type can be
+ // cv-qualified. For now, kernel name types are required to be class types
+ // and that they may be cv-qualified. The following issue requests
+ // clarification from the SYCL WG.
+ // https://github.com/KhronosGroup/SYCL-Docs/issues/568
+ S.Diag(Loc, diag::warn_sycl_kernel_name_not_a_class_type) << KernelName;
+ SourceLocation DeclTypeLoc = SourceLocationForType(KernelName);
+ if (DeclTypeLoc.isValid())
+ S.Diag(DeclTypeLoc, diag::note_entity_declared_at)
+ << KernelName;
+ return true;
+ }
+
+ return false;
+}
+
+void SemaSYCL::CheckSYCLEntryPointFunctionDecl(FunctionDecl *FD) {
+ // Ensure that all attributes present on the declaration are consistent
+ // and warn about any redundant ones.
+ const SYCLKernelEntryPointAttr *SKEPAttr = nullptr;
+ for (auto SAI = FD->specific_attr_begin<SYCLKernelEntryPointAttr>();
+ SAI != FD->specific_attr_end<SYCLKernelEntryPointAttr>();
+ ++SAI) {
+ if (!SKEPAttr) {
+ SKEPAttr = *SAI;
+ continue;
+ }
+ if (!getASTContext().hasSameType(SAI->getKernelName(),
+ SKEPAttr->getKernelName())) {
+ Diag(SAI->getLocation(), diag::err_sycl_entry_point_invalid_redeclaration)
+ << SAI->getKernelName() << SKEPAttr->getKernelName();
+ Diag(SKEPAttr->getLocation(), diag::note_previous_attribute);
+ } else {
+ Diag(SAI->getLocation(),
+ diag::warn_sycl_entry_point_redundant_declaration);
+ Diag(SKEPAttr->getLocation(), diag::note_previous_attribute);
+ }
+ }
+ assert(SKEPAttr && "Missing sycl_kernel_entry_point attribute");
+
+ // Ensure the kernel name type is valid.
+ if (!SKEPAttr->getKernelName()->isDependentType()) {
+ CheckSYCLKernelName(SemaRef, SKEPAttr->getLocation(),
+ SKEPAttr->getKernelName());
+ }
+
+ // Ensure that an attribute present on the previous declaration
+ // matches the one on this declaration.
+ FunctionDecl *PrevFD = FD->getPreviousDecl();
+ if (PrevFD && !PrevFD->isInvalidDecl()) {
+ const auto *PrevSKEPAttr = PrevFD->getAttr<SYCLKernelEntryPointAttr>();
+ if (PrevSKEPAttr) {
+ if (!getASTContext().hasSameType(SKEPAttr->getKernelName(),
+ PrevSKEPAttr->getKernelName())) {
+ Diag(SKEPAttr->getLocation(),
+ diag::err_sycl_entry_point_invalid_redeclaration)
+ << SKEPAttr->getKernelName() << PrevSKEPAttr->getKernelName();
+ Diag(PrevSKEPAttr->getLocation(), diag::note_previous_decl)
+ << PrevFD;;
+ }
+ }
+ }
+
+ if (auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
+ if (!MD->isStatic()) {
+ Diag(SKEPAttr->getLocation(), diag::err_sycl_entry_point_invalid)
+ << /*non-static member function*/0;
+ }
+ }
+ if (FD->isVariadic()) {
+ Diag(SKEPAttr->getLocation(), diag::err_sycl_entry_point_invalid)
+ << /*variadic function*/1;
+ }
+ if (FD->isConsteval()) {
+ Diag(SKEPAttr->getLocation(), diag::err_sycl_entry_point_invalid)
+ << /*consteval function*/5;
+ } else if (FD->isConstexpr()) {
+ Diag(SKEPAttr->getLocation(), diag::err_sycl_entry_point_invalid)
+ << /*constexpr function*/4;
+ }
+ if (FD->isNoReturn()) {
+ Diag(SKEPAttr->getLocation(), diag::err_sycl_entry_point_invalid)
+ << /*noreturn function*/6;
+ }
+
+ if (!FD->getReturnType()->isVoidType()) {
+ Diag(SKEPAttr->getLocation(), diag::err_sycl_entry_point_return_type);
+ }
+
+ if (!FD->isInvalidDecl() && !FD->isTemplated()) {
+ const SYCLKernelInfo *SKI =
+ getASTContext().findSYCLKernelInfo(SKEPAttr->getKernelName());
+ if (SKI) {
+ if (!declaresSameEntity(FD, SKI->getKernelEntryPointDecl())) {
+ // FIXME: This diagnostic should include the origin of the kernel
+ // FIXME: names; not just the locations of the conflicting declarations.
+ Diag(FD->getLocation(), diag::err_sycl_kernel_name_conflict);
+ Diag(SKI->getKernelEntryPointDecl()->getLocation(),
+ diag::note_previous_declaration);
+ }
+ } else {
+ getASTContext().registerSYCLEntryPointFunction(FD);
+ }
+ }
+}
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 5e7a3c8484c88f..59e24f885eee4a 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -1550,6 +1550,8 @@ NamedDecl *Sema::ActOnNonTypeTemplateParameter(Scope *S, Declarator &D,
IdResolver.AddDecl(Param);
}
+ ProcessDeclAttributes(S, Param, D);
+
// C++0x [temp.param]p9:
// A default template-argument may be specified for any kind of
// template-parameter that is not a template parameter pack.
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 719bc0d06f5b11..ac5dd7fd16303e 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1134,8 +1134,19 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
// the presence of a sycl_kernel_entry_point attribute, register it so that
// associated metadata is recreated.
if (FD->hasAttr<SYCLKernelEntryPointAttr>()) {
+ const auto *SKEPAttr = FD->getAttr<SYCLKernelEntryPointAttr>();
ASTContext &C = Reader.getContext();
- C.registerSYCLEntryPointFunction(FD);
+ const SYCLKernelInfo *SKI =
+ C.findSYCLKernelInfo(SKEPAttr->getKernelName());
+ if (SKI) {
+ if (!declaresSameEntity(FD, SKI->getKernelEntryPointDecl())) {
+ Reader.Diag(FD->getLocation(), diag::err_sycl_kernel_name_conflict);
+ Reader.Diag(SKI->getKernelEntryPointDecl()->getLocation(),
+ diag::note_previous_declaration);
+ }
+ } else {
+ C.registerSYCLEntryPointFunction(FD);
+ }
}
}
diff --git a/clang/test/ASTSYCL/ast-dump-sycl-kernel-entry-point.cpp b/clang/test/ASTSYCL/ast-dump-sycl-kernel-entry-point.cpp
index c351f3b7d03eab..0189cf0402d3a3 100644
--- a/clang/test/ASTSYCL/ast-dump-sycl-kernel-entry-point.cpp
+++ b/clang/test/ASTSYCL/ast-dump-sycl-kernel-entry-point.cpp
@@ -107,14 +107,29 @@ void skep5<KN<5,2>>(long) {
// CHECK: | |-TemplateArgument type 'long'
// CHECK: | `-SYCLKernelEntryPointAttr {{.*}} KN<5, 2>
+// FIXME: C++23 [temp.expl.spec]p12 states:
+// FIXME: ... Similarly, attributes appearing in the declaration of a template
+// FIXME: have no effect on an explicit specialization of that template.
+// FIXME: Clang currently instantiates a function template specialization from
+// FIXME: the function template declaration and links it as a previous
+// FIXME: declaration of an explicit specialization. The instantiated
+// FIXME: declaration includes attributes instantiated from the function
+// FIXME: template declaration. When the instantiated declaration and the
+// FIXME: explicit specialization both specify a sycl_kernel_entry_point
+// FIXME: attribute with different kernel name types, a spurious diagnostic
+// FIXME: is issued. The following test case is incorrectly diagnosed as
+// FIXME: having conflicting kernel name types (KN<5,3> vs the incorrectly
+// FIXME: inherited KN<5,-1>).
+#if 0
template<>
[[clang::sycl_kernel_entry_point(KN<5,3>)]]
void skep5<KN<5,-1>>(long long) {
}
-// CHECK: |-FunctionDecl {{.*}} prev {{.*}} skep5 'void (long long)' explicit_specialization
-// CHECK-NEXT: | |-TemplateArgument type 'KN<5, -1>'
-// CHECK: | |-TemplateArgument type 'long long'
-// CHECK: | `-SYCLKernelEntryPointAttr {{.*}} KN<5, 3>
+// FIXME-CHECK: |-FunctionDecl {{.*}} prev {{.*}} skep5 'void (long long)' explicit_specialization
+// FIXME-CHECK-NEXT: | |-TemplateArgument type 'KN<5, -1>'
+// FIXME-CHECK: | |-TemplateArgument type 'long long'
+// FIXME-CHECK: | `-SYCLKernelEntryPointAttr {{.*}} KN<5, 3>
+#endif
template void skep5<KN<5,4>>(int);
// Checks are located with the primary template declaration above.
diff --git a/clang/test/SemaSYCL/sycl-kernel-entry-point-attr-appertainment.cpp b/clang/test/SemaSYCL/sycl-kernel-entry-point-attr-appertainment.cpp
new file mode 100644
index 00000000000000..5f638061a96408
--- /dev/null
+++ b/clang/test/SemaSYCL/sycl-kernel-entry-point-attr-appertainment.cpp
@@ -0,0 +1,255 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -std=c++17 -fsyntax-only -fsycl-is-device -verify %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -std=c++20 -fsyntax-only -fsycl-is-device -verify %s
+
+// These tests validate appertainment for the sycl_kernel_entry_point attribute.
+
+#if __cplusplus >= 202002L
+// Mock coroutine support.
+namespace std {
+
+template<typename Promise = void>
+struct coroutine_handle {
+ template<typename T>
+ coroutine_handle(const coroutine_handle<T>&);
+ static coroutine_handle from_address(void *addr);
+};
+
+template<typename R, typename... Args>
+struct coroutine_traits {
+ struct suspend_never {
+ bool await_ready() const noexcept;
+ void await_suspend(std::coroutine_handle<>) const noexcept;
+ void await_resume() const noexcept;
+ };
+ struct promise_type {
+ void get_return_object() noexcept;
+ suspend_never initial_suspend() const noexcept;
+ suspend_never final_suspend() const noexcept;
+ void return_void() noexcept;
+ void unhandled_exception() noexcept;
+ };
+};
+
+}
+#endif
+
+// A unique kernel name type is required for each declared kernel entry point.
+template<int, int = 0> struct KN;
+
+
+////////////////////////////////////////////////////////////////////////////////
+// Valid declarations.
+////////////////////////////////////////////////////////////////////////////////
+
+// Function declaration with GNU attribute spelling
+__attribute__((sycl_kernel_entry_point(KN<1>)))
+void ok1();
+
+// Function declaration with Clang attribute spelling.
+[[clang::sycl_kernel_entry_point(KN<2>)]]
+void ok2();
+
+// Function definition.
+[[clang::sycl_kernel_entry_point(KN<3>)]]
+void ok3() {}
+
+// Function template definition.
+template<typename KNT, typename T>
+[[clang::sycl_kernel_entry_point(KNT)]]
+void ok4(T) {}
+
+// Function template explicit specialization.
+template<>
+[[clang::sycl_kernel_entry_point(KN<4,1>)]]
+void ok4<KN<4,1>>(int) {}
+
+// Function template explicit instantiation.
+template void ok4<KN<4,2>, long>(long);
+
+namespace NS {
+// Function declaration at namespace scope.
+[[clang::sycl_kernel_entry_point(KN<5>)]]
+void ok5();
+}
+
+struct S6 {
+ // Static member function declaration.
+ [[clang::sycl_kernel_entry_point(KN<6>)]]
+ static void ok6();
+};
+
+// Dependent friend function.
+template<typename KNT>
+struct S7 {
+ [[clang::sycl_kernel_entry_point(KNT)]]
+ friend void ok7(S7) {}
+};
+void test_ok7() {
+ ok7(S7<KN<7>>{});
+}
+
+// The sycl_kernel_entry_point attribute must match across declarations and
+// cannot be added for the first time after a definition.
+[[clang::sycl_kernel_entry_point(KN<8>)]]
+void ok8();
+[[clang::sycl_kernel_entry_point(KN<8>)]]
+void ok8();
+[[clang::sycl_kernel_entry_point(KN<9>)]]
+void ok9();
+void ok9() {}
+void ok10();
+[[clang::sycl_kernel_entry_point(KN<10>)]]
+void ok10() {}
+
+using VOID = void;
+[[c...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
erichkeane
reviewed
Dec 18, 2024
| } else if (FSI->isCoroutine()) { | ||
| Diag(FD->getAttr<SYCLKernelEntryPointAttr>()->getLocation(), | ||
| diag::err_sycl_entry_point_invalid) | ||
| << /*coroutine*/ 7; |
There was a problem hiding this comment.
When these get this high, an enum typically makes sense instead of comments (which get lost/etc).
There was a problem hiding this comment.
Can you point me to an example? I haven't been able to find one. It doesn't look like there is a way to declare an enum with the diagnostic definition. Without the ability to do so, I don't see how adding an intermediate enum somewhere else improves the situation.
There was a problem hiding this comment.
Yes, there isn't a convenient way of doing so in the diagnostics engine unfortunately. I'd love for us to have that! But typically we just define it elsewhere. For large lists like this it can make it significantly more readable, and much less likely to not be updated because of a copy/paste error (which happens OFTEN).
See AttributeArgumentNType and AttributeDeclKind for some examples, but they are kind of sprinkled in quite a few places, Richard had me do some a few times IIRC for function multiversioning. Sema::checkTargetVersionAttr has some more local versions of it.
There was a problem hiding this comment.
Thanks for pointing me to those examples. For both AttributeArgumentNType and AttributeDeclKind, the production of the diagnostic is more complicated. For the first case, the diagnostic is produced in a lambda expression and the %select index is passed as an argument. For the second case, the %select index is computed by mapping from another enumeration using a switch statement. An enum definitely seems warranted for those cases. Similarly, for Sema::checkTargetVersionAttr, there are multiple %select indices involved in each diagnostic and the local enumeration does help with readability.
For simple diagnostics, where the %select index doesn't require computation and where the same index values are not used in multiple places, I think indirection through an enumeration isn't helpful, so I haven't made a change. If you still think a change is warranted, perhaps we can get a second opinion.
There was a problem hiding this comment.
I still disagree strongly. Any time where there are more than 2-3 options, and are used in more than 1 or 2 places, it vastly improves readability and prevents bugs of printing the wrong thing. We can have a second opinion from Aaron, but this is something pretty important to me as code-owner.
There was a problem hiding this comment.
It's a judgement call either way, IMO. Personally, I dislike using one-shot enumerations because they're overkill for what they're used for. The in-line comment saying "this value corresponds to that selection" is plenty to keep the code readable. However, I do like using enumerations when the enum is used for selecting the diagnostic and some other purpose. e.g.,
AwesomeSauce Result = CheckSomeCondition(Whatever);
if (Result != AwesomeSauce::Good)
Diag(Loc, diag::dont_do_that) << Result;
else
Diag(Loc, diag::congrats) << AwesomeSauce::Fantastic;
In this specific case, I lean towards not needing the enumeration because then we have to figure out where that lives (Are we making Sema even more complicated because the diagnostics are in multiple files? Are we adding a new header? Something else?), and that will lead to inconsistency as we add new diagnostics because some folks will add unscoped enums, others will use scoped enums, and the enums will likely live in various different places in the source.
That said, coming up with some tablegen syntax that allows someone to associate enumerations with the diagnostic definition itself would be a pretty interesting idea to explore because then we'd get something that could be consistently applied and doesn't require as much maintenance and review effort. e.g.,
def err_sycl_entry_point_invalid : Error<
"'sycl_kernel_entry_point' attribute cannot be applied to a"
" %enum_select{"
"%enum{NonStaticMemberFunc}non-static member function|"
"%enum{VariadicFunc}variadic function|"
"%enum{DeletedFunc}deleted function|"
"%enum{DefaultedFunc}defaulted function|...<you get the idea>}0">;
that code then be used like:
Diag(Loc, diag::err_sycl_entry_point_invalid) << diag::err_sycl_entry_point_invalid::NonStaticMemberFunction;
(Though I think we may want to find some better place for the enum to live so that uses in calls to Diag() are more succinct. Obviously there's a lot of design work left to be done with the idea.)
There was a problem hiding this comment.
Thanks, @AaronBallman. I like the suggested tablegen approach and would be happy to use that if someone (other than me) implemented it!
I share your perspective on one-shot enumerations. I'll leave the code as is for now. @erichkeane, as a compromise, I would be happy to split this diagnostic into eight distinct ones to avoid the situation all together. I don't think we gain much by using a single diagnostic in the first place. Let me know if you would prefer that.
There was a problem hiding this comment.
I'd prefer we don't split into separate diagnostics; that increases overhead and maintenance burden for very little gain IMO. We generally prefer using %select when possible (it makes it easier to reword diagnostics, fix typos, localize, etc and it avoids adding extra strings to the binary when string merging isn't possible).
clang/lib/Sema/SemaTemplate.cpp
Outdated
| @@ -1550,6 +1550,8 @@ NamedDecl *Sema::ActOnNonTypeTemplateParameter(Scope *S, Declarator &D, | |||
| IdResolver.AddDecl(Param); | |||
| } | |||
|
|
|||
| ProcessDeclAttributes(S, Param, D); | |||
There was a problem hiding this comment.
Hmm... this seems like a concerning addition. I think I want @AaronBallman to take a look at this, this has implications on appertainment/etc that I don't terribly understand, so I thought this is something we only do in places the standard suggests.
There was a problem hiding this comment.
This was needed to ensure diagnostics were issued for a few of the tests. I think this was a simple omission. I appreciate wanting other eyes on it though and we could move this to a separate PR. Note that this checks attributes on the types of non-type template arguments; from the C++ standard perspective, this is like any other type specifier.
There was a problem hiding this comment.
Still want Aaron to check on this.
There was a problem hiding this comment.
We definitely need some changes because we drop the attributes on the floor: https://godbolt.org/z/oEr7bneaE
However, this is interesting:
https://eel.is/c++draft/temp#nt:template-parameter
https://eel.is/c++draft/dcl.fct#nt:parameter-declaration
So I would expect us to handle NTTPs the same as other parameter declarations. But... are NTTP's a parameter? If so, our Attr.td and corresponding tablegen might need some updates to distinguish between function parameters and non-function parameters because of things like https://eel.is/c++draft/dcl.attr.depend or https://eel.is/c++draft/dcl.attr.indet because they're specific about function parameters.
So I don't think this code is wrong, but I do think we may need to take another look at how we process NTTPs because I would have expected them to use the parameter declaration parsing code, which should automatically handle processing attributes correctly.
There was a problem hiding this comment.
Thanks, @AaronBallman.
See also P3324 (Attributes for namespace aliases, template parameters, and lambda captures) which would clarify the situation from the perspective of the C++ standard.
What I'll do for now is to move this change to a separate PR and annotate the impacted test (bad18() in clang/test/SemaSYCL/sycl-kernel-entry-point-attr-appertainment.cpp) with FIXME comments. We can then pursue any additional changes needed via that other PR at our leisure.
There was a problem hiding this comment.
I think that makes sense, thanks!
tahonermann
commented
Dec 18, 2024
| } else if (FSI->isCoroutine()) { | ||
| Diag(FD->getAttr<SYCLKernelEntryPointAttr>()->getLocation(), | ||
| diag::err_sycl_entry_point_invalid) | ||
| << /*coroutine*/ 7; |
There was a problem hiding this comment.
Can you point me to an example? I haven't been able to find one. It doesn't look like there is a way to declare an enum with the diagnostic definition. Without the ability to do so, I don't see how adding an intermediate enum somewhere else improves the situation.
clang/lib/Sema/SemaTemplate.cpp
Outdated
| @@ -1550,6 +1550,8 @@ NamedDecl *Sema::ActOnNonTypeTemplateParameter(Scope *S, Declarator &D, | |||
| IdResolver.AddDecl(Param); | |||
| } | |||
|
|
|||
| ProcessDeclAttributes(S, Param, D); | |||
There was a problem hiding this comment.
This was needed to ensure diagnostics were issued for a few of the tests. I think this was a simple omission. I appreciate wanting other eyes on it though and we could move this to a separate PR. Note that this checks attributes on the types of non-type template arguments; from the C++ standard perspective, this is like any other type specifier.
…ction. Registration of a function as a SYCL kernel entry point when the kernel name provided for the `sycl_kernel_entry_point` attribute is invalid could lead to later errors (e.g., when generating the offload kernel) if the function declaration is not somehow marked as invalid so that such later attempts can be skipped. Most attributes, and the `sycl_kernel_entry_point` attribute specifically, should not affect overload resolution. Marking a function declaration as invalid with `setInvalidDecl()` causes overload resolution to skip the declaration, so use of it is not correct for marking function delcarations with invalid `sycl_kernel_entry_point` attributes. Dropping an invalid attribute with `dropAttr()` would similarly impact debugging and diagnostics generation and is therefore not a good option. This change allows instances of the `sycl_kernel_entry_point` attribute to be marked as invalid and adds checks to avoid SYCL kernel registration for functions with an attribute marked as invalid. Additional tests are provided to validate that the `sycl_kernel_entry_point` attribute is inherited and that non-dependent invalid uses of the attribute in dependent contexts are eagerly diagnosed without requiring an instantiation.
…bdas, and dependent return types. This change addresses issues with diagnostics generation for various uses of the sycl_kernel_entry_point attribute: - Missing diagnostics for deleted or defaulted functions declared in a dependent context. - Missing diagnostics for lambda expressions. - Spurious diagnostics for functions with a dependent return type; a diagnostic was previously issued complaining of a non-void type. - Missing diagnostics for functions with a deduced return type.
|
@erichkeane, thank you for the review comments so far. I think I've addressed or responded to all of them and I look forward to your next round of review. I don't expect you to see this message until January 6th or so, so I hope you are enjoying, or have enjoyed, the holidays! |
… non-deduced void return type requirement.
The C++ standard is not clear whether attributes are permitted in the declarations of non-type template parameters. Previous commits added code to recognize and instantiate attributes in these contexts, but concerns were raised that more changes might be needed. This change removes the previously added support pending clarification from WG21. See also P3324 (Attributes for namespace aliases, template parameters, and lambda captures).
erichkeane
approved these changes
Jan 9, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/14377 Here is the relevant piece of the build log for the reference |
|
Hi @tahonermann do you have an idea on why this is failing? |
|
This must have crossed in the air, we are now rejecting std attributes spelled there. See 1a73654 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/15300 Here is the relevant piece of the build log for the reference |
|
Can someone more familiar with this push a fix? That would be much appreciated. Thanks! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/10186 Here is the relevant piece of the build log for the reference |
|
Thanks for the link to that commit, @erichkeane, I'll push a fix shortly. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/11135 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/12519 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/10014 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/10356 Here is the relevant piece of the build log for the reference |
|
A PR to fix the broken test has now been posted at #122375. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/16621 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/15876 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/19281 Here is the relevant piece of the build log for the reference |
BaiXilin
pushed a commit
to BaiXilin/llvm-fix-vnni-instr-types
that referenced
this pull request
Jan 12, 2025
…lvm#120327) The `sycl_kernel_entry_point` attribute is used to declare a function that defines a pattern for an offload kernel entry point. The attribute requires a single type argument that specifies a class type that meets the requirements for a SYCL kernel name as described in section 5.2, "Naming of kernels", of the SYCL 2020 specification. A unique kernel name type is required for each function declared with the attribute. The attribute may not first appear on a declaration that follows a definition of the function. The function is required to have a non-deduced `void` return type. The function must not be a non-static member function, be deleted or defaulted, be declared with the `constexpr` or `consteval` specifiers, be declared with the `[[noreturn]]` attribute, be a coroutine, or accept variadic arguments. Diagnostics are not yet provided for the following: - Use of a type as a kernel name that does not satisfy the forward declarability requirements specified in section 5.2, "Naming of kernels", of the SYCL 2020 specification. - Use of a type as a parameter of the attributed function that does not satisfy the kernel parameter requirements specified in section 4.12.4, "Rules for parameter passing to kernels", of the SYCL 2020 specification (each such function parameter constitutes a kernel parameter). - Use of language features that are not permitted in device functions as specified in section 5.4, "Language restrictions for device functions", of the SYCL 2020 specification. There are several issues noted by various FIXME comments. - The diagnostic generated for kernel name conflicts needs additional work to better detail the relevant source locations; such as the location of each declaration as well as the original source of each kernel name. - A number of the tests illustrate spurious errors being produced due to attributes that appertain to function templates being instantiated too early (during overload resolution as opposed to after an overload is selected). Included changes allow the `SYCLKernelEntryPointAttr` attribute to be marked as invalid if a `sycl_kernel_entry_point` attribute is used incorrectly. This is intended to prevent trying to emit an offload kernel entry point without having to mark the associated function as invalid since doing so would affect overload resolution; which this attribute should not do. Unfortunately, Clang eagerly instantiates attributes that appertain to functions with the result that errors might be issued for function declarations that are never selected by overload resolution. Tests have been added to demonstrate this. Further work will be needed to address these issues (for this and other attributes).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
sycl_kernel_entry_pointattribute is used to declare a function that defines a pattern for an offload kernel entry point. The attribute requires a single type argument that specifies a class type that meets the requirements for a SYCL kernel name as described in section 5.2, "Naming of kernels", of the SYCL 2020 specification. A unique kernel name type is required for each function declared with the attribute. The attribute may not first appear on a declaration that follows a definition of the function. The function is required to have avoidreturn type. The function must not be a non-static member function, be deleted or defaulted, be declared with theconstexprorconstevalspecifiers, be declared with the[[noreturn]]attribute, be a coroutine, or accept variadic arguments.Diagnostics are not yet provided for the following:
There are several issues noted by various FIXME comments.