Skip to content

Commit 8a35cf0

Browse files
committed
[SYCL] Fix check for reqd_sub_group_size attribute mismatches
Fixed an issue with comparing pointers to Attr objects rather than values passed to the attribute in source code. Refactored IntelReqdSubGroupSizeAttr class to store evaluated value of attribute value for further uses to avoid evaluating it each time we need that value to check something. The bug were uncovered after #1778 landed, but it had been introduced in #1807
1 parent bf8493c commit 8a35cf0

File tree

5 files changed

+34
-21
lines changed

5 files changed

+34
-21
lines changed

clang/include/clang/Basic/Attr.td

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1289,11 +1289,28 @@ def LoopUnrollHint : InheritableAttr {
12891289

12901290
def IntelReqdSubGroupSize: InheritableAttr {
12911291
let Spellings = [GNU<"intel_reqd_sub_group_size">, CXX11<"cl", "intel_reqd_sub_group_size">];
1292-
let Args = [ExprArgument<"SubGroupSize">];
1292+
let Args = [ExprArgument<"SubGroupSizeExpr">];
12931293
let Subjects = SubjectList<[Function, CXXMethod], ErrorDiag>;
12941294
let Documentation = [IntelReqdSubGroupSizeDocs];
12951295
let LangOpts = [OpenCL, SYCLIsDevice, SYCLIsHost];
12961296
let PragmaAttributeSupport = 0;
1297+
let AdditionalMembers = [{
1298+
private:
1299+
int SubGroupSize = 0;
1300+
1301+
public:
1302+
int getSubGroupSizeEvaluated(ASTContext &Ctx) {
1303+
if (SubGroupSize != 0)
1304+
return SubGroupSize;
1305+
llvm::APSInt Val(32);
1306+
bool Succeedded =
1307+
this->getSubGroupSizeExpr()->isIntegerConstantExpr(Val, Ctx);
1308+
assert(Succeedded && "expression must be constant integer");
1309+
SubGroupSize = Val.getSExtValue();
1310+
assert(SubGroupSize >= 0 && "invalid value of the attribute");
1311+
return SubGroupSize;
1312+
}
1313+
}];
12971314
}
12981315

12991316
// This attribute is both a type attribute, and a declaration attribute (for

clang/lib/CodeGen/CodeGenFunction.cpp

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -565,16 +565,10 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD,
565565
Fn->setMetadata("reqd_work_group_size", llvm::MDNode::get(Context, AttrMDArgs));
566566
}
567567

568-
if (const IntelReqdSubGroupSizeAttr *A =
569-
FD->getAttr<IntelReqdSubGroupSizeAttr>()) {
570-
llvm::APSInt ArgVal(32);
568+
if (IntelReqdSubGroupSizeAttr *A = FD->getAttr<IntelReqdSubGroupSizeAttr>()) {
571569
llvm::LLVMContext &Context = getLLVMContext();
572-
bool IsValid = A->getSubGroupSize()->isIntegerConstantExpr(
573-
ArgVal, FD->getASTContext());
574-
assert(IsValid && "Not an integer constant expression");
575-
(void)IsValid;
576-
llvm::Metadata *AttrMDArgs[] = {
577-
llvm::ConstantAsMetadata::get(Builder.getInt32(ArgVal.getSExtValue()))};
570+
llvm::Metadata *AttrMDArgs[] = {llvm::ConstantAsMetadata::get(
571+
Builder.getInt32(A->getSubGroupSizeEvaluated(FD->getASTContext())))};
578572
Fn->setMetadata("intel_reqd_sub_group_size",
579573
llvm::MDNode::get(Context, AttrMDArgs));
580574
}

clang/lib/Sema/SemaSYCL.cpp

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -308,14 +308,6 @@ static void reportConflictingAttrs(Sema &S, FunctionDecl *F, const Attr *A1,
308308
F->setInvalidDecl();
309309
}
310310

311-
// Returns the signed constant integer value represented by given expression.
312-
static int64_t getIntExprValue(Sema &S, const Expr *E) {
313-
llvm::APSInt Val(32);
314-
bool IsValid = E->isIntegerConstantExpr(Val, S.getASTContext());
315-
assert(IsValid && "expression must be constant integer");
316-
return Val.getSExtValue();
317-
}
318-
319311
class MarkDeviceFunction : public RecursiveASTVisitor<MarkDeviceFunction> {
320312
public:
321313
MarkDeviceFunction(Sema &S)
@@ -1696,15 +1688,16 @@ void Sema::MarkDevice(void) {
16961688
KernelBody ? KernelBody->getAttr<SYCLSimdAttr>() : nullptr;
16971689
if (auto *Existing =
16981690
SYCLKernel->getAttr<IntelReqdSubGroupSizeAttr>()) {
1699-
if (Existing->getSubGroupSize() != Attr->getSubGroupSize()) {
1691+
if (Existing->getSubGroupSizeEvaluated(getASTContext()) !=
1692+
Attr->getSubGroupSizeEvaluated(getASTContext())) {
17001693
Diag(SYCLKernel->getLocation(),
17011694
diag::err_conflicting_sycl_kernel_attributes);
17021695
Diag(Existing->getLocation(), diag::note_conflicting_attribute);
17031696
Diag(Attr->getLocation(), diag::note_conflicting_attribute);
17041697
SYCLKernel->setInvalidDecl();
17051698
}
17061699
} else if (KBSimdAttr &&
1707-
(getIntExprValue(*this, Attr->getSubGroupSize()) != 1)) {
1700+
(Attr->getSubGroupSizeEvaluated(getASTContext()) != 1)) {
17081701
reportConflictingAttrs(*this, KernelBody, KBSimdAttr, Attr);
17091702
} else {
17101703
SYCLKernel->addAttr(A);

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -547,7 +547,7 @@ static void instantiateIntelReqdSubGroupSizeAttr(
547547
// The SubGroupSize expression is a constant expression.
548548
EnterExpressionEvaluationContext Unevaluated(
549549
S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
550-
ExprResult Result = S.SubstExpr(Attr->getSubGroupSize(), TemplateArgs);
550+
ExprResult Result = S.SubstExpr(Attr->getSubGroupSizeExpr(), TemplateArgs);
551551
if (!Result.isInvalid())
552552
S.addIntelReqdSubGroupSizeAttr(New, *Attr, Result.getAs<Expr>());
553553
}

clang/test/SemaSYCL/reqd-sub-group-size-device.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,16 @@ void bar() {
4545
baz();
4646
});
4747
#endif
48+
4849
kernel<class kernel_name5>([]() [[cl::intel_reqd_sub_group_size(2)]] { });
50+
kernel<class kernel_name6>([]() [[cl::intel_reqd_sub_group_size(4)]] { foo(); });
51+
}
52+
53+
[[cl::intel_reqd_sub_group_size(16)]] SYCL_EXTERNAL void B();
54+
[[cl::intel_reqd_sub_group_size(16)]] void A() {
55+
}
56+
[[cl::intel_reqd_sub_group_size(16)]] SYCL_EXTERNAL void B() {
57+
A();
4958
}
5059

5160
#ifdef TRIGGER_ERROR

0 commit comments

Comments
 (0)