Skip to content

[SYCL] Basic diagnostics for the sycl_kernel_entry_point attribute. #120327

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

Merged
merged 10 commits into from
Jan 9, 2025
Merged

[SYCL] Basic diagnostics for the sycl_kernel_entry_point attribute. #120327

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6ed96d3
[SYCL] Basic diagnostics for the sycl_kernel_entry_point attribute.
tahonermann Nov 1, 2024
0b7f6bf
Squash; Fix clang-format complaints.
tahonermann Dec 17, 2024
0e89f39
Avoid attempted invalid registration of a SYCL kernel entry point fun…
tahonermann Dec 23, 2024
5eeff88
Fixes to address the first round of code review comments from Erich.
tahonermann Dec 23, 2024
97f58f4
Squash; Fix clang-format complaints. Again.
tahonermann Dec 24, 2024
da651f6
Added tests for dependent and non-dependent hidden friend functions.
tahonermann Dec 24, 2024
c0044ab
Fixes to address issues with diagnostics for templated functions, lam…
tahonermann Dec 24, 2024
1a0898f
Squash; Fix clang-format complaints. Again.
tahonermann Dec 25, 2024
977f4fa
Update sycl_kernel_entry_point attribute documentation to include the…
tahonermann Jan 8, 2025
b30dd84
Remove the fix for attributes in non-type template parameters.
tahonermann Jan 8, 2025
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
10 changes: 10 additions & 0 deletions clang/include/clang/AST/ASTContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
//===--------------------------------------------------------------------===//
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Basic/DiagnosticGroups.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">;
Expand Down
23 changes: 23 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Sema/SemaSYCL.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions clang/lib/AST/ASTContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
30 changes: 29 additions & 1 deletion clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
#include "clang/Sema/SemaOpenMP.h"
#include "clang/Sema/SemaPPC.h"
#include "clang/Sema/SemaRISCV.h"
#include "clang/Sema/SemaSYCL.h"
#include "clang/Sema/SemaSwift.h"
#include "clang/Sema/SemaWasm.h"
#include "clang/Sema/Template.h"
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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).

Expand Down Expand Up @@ -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;
Copy link
Collaborator

@erichkeane erichkeane Dec 18, 2024

Choose a reason for hiding this comment

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

When these get this high, an enum typically makes sense instead of comments (which get lost/etc).

Copy link
Contributor Author

@tahonermann tahonermann Dec 18, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@erichkeane erichkeane Dec 18, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tahonermann tahonermann Dec 24, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@erichkeane erichkeane Jan 2, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@AaronBallman AaronBallman Jan 8, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tahonermann tahonermann Jan 8, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@AaronBallman AaronBallman Jan 9, 2025

Choose a reason for hiding this comment

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

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

}
}

{
// Do not call PopExpressionEvaluationContext() if it is a lambda because
// one is already popped when finishing the lambda in BuildLambdaExpr().
Expand Down
123 changes: 123 additions & 0 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -206,3 +208,124 @@ 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);
}
}
}
2 changes: 2 additions & 0 deletions clang/lib/Sema/SemaTemplate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1550,6 +1550,8 @@ NamedDecl *Sema::ActOnNonTypeTemplateParameter(Scope *S, Declarator &D,
IdResolver.AddDecl(Param);
}

ProcessDeclAttributes(S, Param, D);
Copy link
Collaborator

@erichkeane erichkeane Dec 18, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tahonermann tahonermann Dec 18, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@erichkeane erichkeane Jan 2, 2025

Choose a reason for hiding this comment

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

Still want Aaron to check on this.

Copy link
Collaborator

@AaronBallman AaronBallman Jan 8, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tahonermann tahonermann Jan 8, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@AaronBallman AaronBallman Jan 9, 2025

Choose a reason for hiding this comment

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

I think that makes sense, thanks!


// 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.
Expand Down
12 changes: 11 additions & 1 deletion clang/lib/Serialization/ASTReaderDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1134,8 +1134,18 @@ 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);
}
}
}

Expand Down
23 changes: 19 additions & 4 deletions clang/test/ASTSYCL/ast-dump-sycl-kernel-entry-point.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading
Loading