Skip to content

[SYCL] Add template parameter support for intel_reqd_sub_group_size a… #1807

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 5 commits into from
Jun 7, 2020
Merged

[SYCL] Add template parameter support for intel_reqd_sub_group_size a… #1807

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d96007d
[SYCL] Add template parameter support for intel_reqd_sub_group_size a…
smanna12 Jun 2, 2020
60c7b00
[SYCL] Add template parameter support for intel_reqd_sub_group_size a…
smanna12 Jun 4, 2020
57d61ce
[SYCL] Add template parameter support for intel_reqd_sub_group_size a…
smanna12 Jun 4, 2020
462320e
[SYCL] Add template parameter support for intel_reqd_sub_group_size a…
smanna12 Jun 4, 2020
257f6ee
[SYCL] Add template parameter support for intel_reqd_sub_group_size a…
smanna12 Jun 7, 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
2 changes: 1 addition & 1 deletion clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1262,7 +1262,7 @@ def LoopUnrollHint : InheritableAttr {

def IntelReqdSubGroupSize: InheritableAttr {
let Spellings = [GNU<"intel_reqd_sub_group_size">, CXX11<"cl", "intel_reqd_sub_group_size">];
let Args = [UnsignedArgument<"SubGroupSize">];
let Args = [ExprArgument<"SubGroupSize">];
let Subjects = SubjectList<[Function, CXXMethod], ErrorDiag>;
let Documentation = [IntelReqdSubGroupSizeDocs];
let LangOpts = [OpenCL, SYCLIsDevice, SYCLIsHost];
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9917,6 +9917,11 @@ class Sema final {

bool checkNSReturnsRetainedReturnType(SourceLocation loc, QualType type);

// addIntelReqdSubGroupSizeAttr - Adds an intel_reqd_sub_group_size attribute
Copy link
Contributor

@bader bader Jun 6, 2020

Choose a reason for hiding this comment

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

Suggested change
// addIntelReqdSubGroupSizeAttr - Adds an intel_reqd_sub_group_size attribute
// Adds an intel_reqd_sub_group_size attribute

Copy link
Contributor Author

@smanna12 smanna12 Jun 7, 2020

Choose a reason for hiding this comment

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

Done

// to a particular declaration.
void addIntelReqdSubGroupSizeAttr(Decl *D, const AttributeCommonInfo &CI,
Expr *E);

//===--------------------------------------------------------------------===//
// C++ Coroutines TS
//
Expand Down
8 changes: 7 additions & 1 deletion clang/lib/CodeGen/CodeGenFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,8 +574,14 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD,

if (const IntelReqdSubGroupSizeAttr *A =
FD->getAttr<IntelReqdSubGroupSizeAttr>()) {
llvm::APSInt ArgVal(32);
llvm::LLVMContext &Context = getLLVMContext();
bool IsValid = A->getSubGroupSize()->isIntegerConstantExpr(
ArgVal, FD->getASTContext());
assert(IsValid && "Not an integer constant expression");
(void)IsValid;
llvm::Metadata *AttrMDArgs[] = {
llvm::ConstantAsMetadata::get(Builder.getInt32(A->getSubGroupSize()))};
llvm::ConstantAsMetadata::get(Builder.getInt32(ArgVal.getSExtValue()))};
Fn->setMetadata("intel_reqd_sub_group_size",
llvm::MDNode::get(Context, AttrMDArgs));
}
Expand Down
42 changes: 28 additions & 14 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2980,27 +2980,41 @@ static void handleWorkGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) {
}

// Handles intel_reqd_sub_group_size.
void Sema::addIntelReqdSubGroupSizeAttr(Decl *D,
const AttributeCommonInfo &Attr,
Expr *E) {
if (!E)
return;

if (!E->isInstantiationDependent()) {
llvm::APSInt ArgVal(32);
if (!E->isIntegerConstantExpr(ArgVal, getASTContext())) {
Diag(E->getExprLoc(), diag::err_attribute_argument_type)
<< Attr.getAttrName() << AANT_ArgumentIntegerConstant
<< E->getSourceRange();
return;
}
int32_t ArgInt = ArgVal.getSExtValue();
if (ArgInt <= 0) {
Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer)
<< Attr.getAttrName() << /*positive*/ 0;
return;
}
}

D->addAttr(::new (Context) IntelReqdSubGroupSizeAttr(Context, Attr, E));
}

static void handleSubGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) {
if (S.LangOpts.SYCLIsHost)
return;

uint32_t SGSize;
const Expr *E = AL.getArgAsExpr(0);
if (!checkUInt32Argument(S, AL, E, SGSize))
return;
if (SGSize == 0) {
S.Diag(AL.getLoc(), diag::err_attribute_argument_is_zero)
<< AL << E->getSourceRange();
return;
}
Expr *E = AL.getArgAsExpr(0);

IntelReqdSubGroupSizeAttr *Existing =
D->getAttr<IntelReqdSubGroupSizeAttr>();
if (Existing && Existing->getSubGroupSize() != SGSize)
if (D->getAttr<IntelReqdSubGroupSizeAttr>())
S.Diag(AL.getLoc(), diag::warn_duplicate_attribute) << AL;

D->addAttr(::new (S.Context)
IntelReqdSubGroupSizeAttr(S.Context, AL, SGSize));
S.addIntelReqdSubGroupSizeAttr(D, AL, E);
}

// Handles num_simd_work_items.
Expand Down
18 changes: 17 additions & 1 deletion clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,17 @@ static void instantiateSYCLIntelPipeIOAttr(
S.addSYCLIntelPipeIOAttr(New, *Attr, Result.getAs<Expr>());
}

static void instantiateIntelReqdSubGroupSizeAttr(
Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
const IntelReqdSubGroupSizeAttr *Attr, Decl *New) {
// The SubGroupSize expression is a constant expression.
EnterExpressionEvaluationContext Unevaluated(
S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
ExprResult Result = S.SubstExpr(Attr->getSubGroupSize(), TemplateArgs);
if (!Result.isInvalid())
S.addIntelReqdSubGroupSizeAttr(New, *Attr, Result.getAs<Expr>());
}

void Sema::InstantiateAttrsForDecl(
const MultiLevelTemplateArgumentList &TemplateArgs, const Decl *Tmpl,
Decl *New, LateInstantiatedAttrVec *LateAttrs,
Expand Down Expand Up @@ -721,7 +732,12 @@ void Sema::InstantiateAttrs(const MultiLevelTemplateArgumentList &TemplateArgs,
instantiateSYCLIntelPipeIOAttr(*this, TemplateArgs, SYCLIntelPipeIO, New);
continue;
}

if (const auto *IntelReqdSubGroupSize =
dyn_cast<IntelReqdSubGroupSizeAttr>(TmplAttr)) {
instantiateIntelReqdSubGroupSizeAttr(*this, TemplateArgs,
IntelReqdSubGroupSize, New);
continue;
}
// Existing DLL attribute on the instantiation takes precedence.
if (TmplAttr->getKind() == attr::DLLExport ||
TmplAttr->getKind() == attr::DLLImport) {
Expand Down
12 changes: 11 additions & 1 deletion clang/test/CodeGenSYCL/reqd-sub-group-size.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ class Functor {
}
};

template <int SIZE>
class Functor5 {
public:
[[cl::intel_reqd_sub_group_size(SIZE)]] void operator()() {}
};

template <typename name, typename Func>
__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
kernelFunc();
Expand All @@ -28,12 +34,16 @@ void bar() {

kernel<class kernel_name3>(
[]() [[cl::intel_reqd_sub_group_size(4)]] {});

Functor5<2> f5;
kernel<class kernel_name4>(f5);
}

// CHECK: define spir_kernel void @{{.*}}kernel_name1() {{.*}} !intel_reqd_sub_group_size ![[SGSIZE16:[0-9]+]]
// CHECK: define spir_kernel void @{{.*}}kernel_name2() {{.*}} !intel_reqd_sub_group_size ![[SGSIZE8:[0-9]+]]
// CHECK: define spir_kernel void @{{.*}}kernel_name3() {{.*}} !intel_reqd_sub_group_size ![[SGSIZE4:[0-9]+]]
// CHECK: define spir_kernel void @{{.*}}kernel_name4() {{.*}} !intel_reqd_sub_group_size ![[SGSIZE2:[0-9]+]]
// CHECK: ![[SGSIZE16]] = !{i32 16}
// CHECK: ![[SGSIZE8]] = !{i32 8}
// CHECK: ![[SGSIZE4]] = !{i32 4}

// CHECK: ![[SGSIZE2]] = !{i32 2}
2 changes: 1 addition & 1 deletion clang/test/SemaOpenCL/invalid-kernel-attrs.cl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ kernel __attribute__((reqd_work_group_size(1,0,2))) void kernel12(){} // expecte
kernel __attribute__((reqd_work_group_size(0,1,2))) void kernel13(){} // expected-error {{'reqd_work_group_size' attribute must be greater than 0}}

__attribute__((intel_reqd_sub_group_size(8))) void kernel14(){} // expected-error {{attribute 'intel_reqd_sub_group_size' can only be applied to an OpenCL kernel}}
kernel __attribute__((intel_reqd_sub_group_size(0))) void kernel15(){} // expected-error {{'intel_reqd_sub_group_size' attribute must be greater than 0}}
kernel __attribute__((intel_reqd_sub_group_size(0))) void kernel15(){} // expected-error {{'intel_reqd_sub_group_size' attribute requires a positive integral compile time constant expression}}
Copy link
Contributor

@erichkeane erichkeane Jun 4, 2020

Choose a reason for hiding this comment

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

This has changed because you're now treating this as a signed value, whereas the previous code treated it as unsigned, and thus a different error message makes sense.

Please add a test with a negative value as well to make sure that doesn't change again. I realize you have one in the template version, but I'd like one right next to this one to show intent.

Copy link
Contributor Author

@smanna12 smanna12 Jun 4, 2020

Choose a reason for hiding this comment

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

I have added a test with a negative value. But it seems like changing again. I will try to add inserting of empty line to fix the clang-format check error.

kernel __attribute__((intel_reqd_sub_group_size(8))) __attribute__((intel_reqd_sub_group_size(16))) void kernel16() {} //expected-warning{{attribute 'intel_reqd_sub_group_size' is already applied with different parameters}}

__kernel __attribute__((work_group_size_hint(8,-16,32))) void neg1() {} //expected-error{{'work_group_size_hint' attribute requires a non-negative integral compile time constant expression}}
Expand Down
9 changes: 6 additions & 3 deletions clang/test/SemaSYCL/reqd-sub-group-size-device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,11 @@ void bar() {
}

// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name1
// CHECK: IntelReqdSubGroupSizeAttr {{.*}} 16
// CHECK: IntelReqdSubGroupSizeAttr {{.*}}
// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name2
// CHECK: IntelReqdSubGroupSizeAttr {{.*}} 4
// CHECK: IntelReqdSubGroupSizeAttr {{.*}}
// CHECK-NEXT: IntegerLiteral{{.*}}4{{$}}
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name5
// CHECK: IntelReqdSubGroupSizeAttr {{.*}} 2
// CHECK: IntelReqdSubGroupSizeAttr {{.*}}
// CHECK-NEXT: IntegerLiteral{{.*}}2{{$}}
25 changes: 25 additions & 0 deletions clang/test/SemaSYCL/sycl-device-reqd-sub-group-size-template.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// RUN: %clang_cc1 -fsycl -fsycl-is-device -fsyntax-only -verify -pedantic %s
// RUN: %clang_cc1 -fsycl -fsycl-is-device -fsyntax-only -ast-dump -verify -pedantic %s | FileCheck %s
Copy link
Contributor

@bader bader Jun 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't the second command cover both diagnostics and AST checks?

Copy link
Contributor Author

@smanna12 smanna12 Jun 7, 2020

Choose a reason for hiding this comment

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

Thanks @bader. Yes, the second command covers both diagnostics and AST checks.
First RUN command is not needed and this is removed now.


// Test that checkes template parameter support for 'intel_reqd_sub_group_size' attribute on sycl device.

template <int SIZE>
class KernelFunctor {
public:
//expected-error@+1{{'intel_reqd_sub_group_size' attribute requires a positive integral compile time constant expression}}
[[cl::intel_reqd_sub_group_size(SIZE)]] void operator()() {}
};

int main() {
//expected-note@+1{{in instantiation of template class 'KernelFunctor<-1>' requested here}}
KernelFunctor<-1>();
// no error expected
KernelFunctor<10>();
}

// CHECK: ClassTemplateDecl {{.*}} {{.*}} KernelFunctor
// CHECK: ClassTemplateSpecializationDecl {{.*}} {{.*}} class KernelFunctor definition
// CHECK: CXXRecordDecl {{.*}} {{.*}} implicit class KernelFunctor
// CHECK: IntelReqdSubGroupSizeAttr {{.*}}
// CHECK: SubstNonTypeTemplateParmExpr {{.*}}
// CHECK-NEXT: IntegerLiteral{{.*}}10{{$}}