Skip to content

Commit 1b9e51b

Browse files
smanna12smanna12
authored
[SYCL] Refactor reqd_work_group_size attribute implementation (#5782)
This patch 1. refactors SYCL kernel function attribute reqd_work_group_size to better fit for community standards. 2. refactors the way we handled duplicate attributes logic with when present on a given declaration. 3. refactors the way we handled mutually exclusive attributes logic with when present on a given declaration with reqd_work_group_size, max_work_group_size, max_global_work_dim, and num_simd_work_items attributes. 5. handles redeclarations or template instantiations properly. 6. stores ConstantExpr into the semantic attribute rather than calling getIntegerConstantExpr() for attribute dimension values. 7. fixes crash with optional arguments on ReqdWorkGroupAttr and adds missing diagnostics for merging cases when present on a given declaration with reqd_work_group_size, max_work_group_size, max_global_work_dim, and num_simd_work_items attributes. 8. updates clang-tidy build after reqd_work_group_size attribute changes. 9. adds tests. This patch fixes #5736, #5735, #5734, #5732, #5731, #3223, #3361, #3175, #3742, and #5733 Signed-off-by: Soumi Manna <[email protected]>
1 parent 01f5b18 commit 1b9e51b

22 files changed

+795
-649
lines changed

‎clang-tools-extra/clang-tidy/altera/SingleWorkItemBarrierCheck.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,8 @@ void SingleWorkItemBarrierCheck::check(const MatchFinder::MatchResult &Result) {
5757
bool IsNDRange = false;
5858
if (MatchedDecl->hasAttr<ReqdWorkGroupSizeAttr>()) {
5959
const auto *Attribute = MatchedDecl->getAttr<ReqdWorkGroupSizeAttr>();
60-
if (*Attribute->getXDimVal(*Result.Context) > 1 ||
61-
*Attribute->getYDimVal(*Result.Context) > 1 ||
62-
*Attribute->getZDimVal(*Result.Context) > 1)
60+
if (*Attribute->getXDimVal() > 1 || *Attribute->getYDimVal() > 1 ||
61+
*Attribute->getZDimVal() > 1)
6362
IsNDRange = true;
6463
}
6564
if (IsNDRange) // No warning if kernel is treated as an NDRange.

‎clang/include/clang/Basic/Attr.td

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3251,17 +3251,20 @@ def ReqdWorkGroupSize : InheritableAttr {
32513251
ExprArgument<"ZDim", /*optional*/1>];
32523252
let Subjects = SubjectList<[Function], ErrorDiag>;
32533253
let AdditionalMembers = [{
3254-
ArrayRef<const Expr *> dimensions() const {
3255-
return {getXDim(), getYDim(), getZDim()};
3256-
}
3257-
Optional<llvm::APSInt> getXDimVal(ASTContext &Ctx) const {
3258-
return getXDim()->getIntegerConstantExpr(Ctx);
3254+
Optional<llvm::APSInt> getXDimVal() const {
3255+
if (const auto *CE = dyn_cast<ConstantExpr>(getXDim()))
3256+
return CE->getResultAsAPSInt();
3257+
return None;
32593258
}
3260-
Optional<llvm::APSInt> getYDimVal(ASTContext &Ctx) const {
3261-
return getYDim()->getIntegerConstantExpr(Ctx);
3259+
Optional<llvm::APSInt> getYDimVal() const {
3260+
if (const auto *CE = dyn_cast<ConstantExpr>(getYDim()))
3261+
return CE->getResultAsAPSInt();
3262+
return None;
32623263
}
3263-
Optional<llvm::APSInt> getZDimVal(ASTContext &Ctx) const {
3264-
return getZDim()->getIntegerConstantExpr(Ctx);
3264+
Optional<llvm::APSInt> getZDimVal() const {
3265+
if (const auto *CE = dyn_cast<ConstantExpr>(getZDim()))
3266+
return CE->getResultAsAPSInt();
3267+
return None;
32653268
}
32663269
}];
32673270
let Documentation = [ReqdWorkGroupSizeAttrDocs];

‎clang/include/clang/Sema/Sema.h

Lines changed: 4 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -10541,9 +10541,6 @@ class Sema final {
1054110541

1054210542
void AddIntelFPGABankBitsAttr(Decl *D, const AttributeCommonInfo &CI,
1054310543
Expr **Exprs, unsigned Size);
10544-
template <typename AttrType>
10545-
void addIntelTripleArgAttr(Decl *D, const AttributeCommonInfo &CI,
10546-
Expr *XDimExpr, Expr *YDimExpr, Expr *ZDimExpr);
1054710544
void AddWorkGroupSizeHintAttr(Decl *D, const AttributeCommonInfo &CI,
1054810545
Expr *XDim, Expr *YDim, Expr *ZDim);
1054910546
WorkGroupSizeHintAttr *
@@ -10640,6 +10637,10 @@ class Sema final {
1064010637
void AddSYCLAddIRAttributesGlobalVariableAttr(Decl *D,
1064110638
const AttributeCommonInfo &CI,
1064210639
MutableArrayRef<Expr *> Args);
10640+
void AddReqdWorkGroupSizeAttr(Decl *D, const AttributeCommonInfo &CI,
10641+
Expr *XDim, Expr *YDim, Expr *ZDim);
10642+
ReqdWorkGroupSizeAttr *
10643+
MergeReqdWorkGroupSizeAttr(Decl *D, const ReqdWorkGroupSizeAttr &A);
1064310644
/// AddAlignedAttr - Adds an aligned attribute to a particular declaration.
1064410645
void AddAlignedAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E,
1064510646
bool IsPackExpansion);
@@ -13736,62 +13737,6 @@ class Sema final {
1373613737
}
1373713738
};
1373813739

13739-
inline Expr *checkMaxWorkSizeAttrExpr(Sema &S, const AttributeCommonInfo &CI,
13740-
Expr *E) {
13741-
assert(E && "Attribute must have an argument.");
13742-
13743-
if (!E->isInstantiationDependent()) {
13744-
llvm::APSInt ArgVal;
13745-
ExprResult ICE = S.VerifyIntegerConstantExpression(E, &ArgVal);
13746-
13747-
if (ICE.isInvalid())
13748-
return nullptr;
13749-
13750-
E = ICE.get();
13751-
13752-
if (ArgVal.isNegative()) {
13753-
S.Diag(E->getExprLoc(),
13754-
diag::warn_attribute_requires_non_negative_integer_argument)
13755-
<< E->getType() << S.Context.UnsignedLongLongTy
13756-
<< E->getSourceRange();
13757-
return E;
13758-
}
13759-
13760-
unsigned Val = ArgVal.getZExtValue();
13761-
if (Val == 0) {
13762-
S.Diag(E->getExprLoc(), diag::err_attribute_argument_is_zero)
13763-
<< CI << E->getSourceRange();
13764-
return nullptr;
13765-
}
13766-
}
13767-
return E;
13768-
}
13769-
13770-
template <typename WorkGroupAttrType>
13771-
void Sema::addIntelTripleArgAttr(Decl *D, const AttributeCommonInfo &CI,
13772-
Expr *XDimExpr, Expr *YDimExpr,
13773-
Expr *ZDimExpr) {
13774-
13775-
assert((XDimExpr && YDimExpr && ZDimExpr) &&
13776-
"argument has unexpected null value");
13777-
13778-
// Accept template arguments for now as they depend on something else.
13779-
// We'll get to check them when they eventually get instantiated.
13780-
if (!XDimExpr->isValueDependent() && !YDimExpr->isValueDependent() &&
13781-
!ZDimExpr->isValueDependent()) {
13782-
13783-
// Save ConstantExpr in semantic attribute
13784-
XDimExpr = checkMaxWorkSizeAttrExpr(*this, CI, XDimExpr);
13785-
YDimExpr = checkMaxWorkSizeAttrExpr(*this, CI, YDimExpr);
13786-
ZDimExpr = checkMaxWorkSizeAttrExpr(*this, CI, ZDimExpr);
13787-
13788-
if (!XDimExpr || !YDimExpr || !ZDimExpr)
13789-
return;
13790-
}
13791-
D->addAttr(::new (Context)
13792-
WorkGroupAttrType(Context, CI, XDimExpr, YDimExpr, ZDimExpr));
13793-
}
13794-
1379513740
/// RAII object that enters a new expression evaluation context.
1379613741
class EnterExpressionEvaluationContext {
1379713742
Sema &Actions;

‎clang/lib/CodeGen/CodeGenFunction.cpp

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -636,22 +636,13 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD,
636636
}
637637

638638
if (const ReqdWorkGroupSizeAttr *A = FD->getAttr<ReqdWorkGroupSizeAttr>()) {
639-
ASTContext &ClangCtx = FD->getASTContext();
640-
Optional<llvm::APSInt> XDimVal = A->getXDimVal(ClangCtx);
641-
Optional<llvm::APSInt> YDimVal = A->getYDimVal(ClangCtx);
642-
Optional<llvm::APSInt> ZDimVal = A->getZDimVal(ClangCtx);
643-
644-
// For a SYCLDevice ReqdWorkGroupSizeAttr arguments are reversed.
645-
if (getLangOpts().SYCLIsDevice)
646-
std::swap(XDimVal, ZDimVal);
647-
639+
// Attributes arguments (first and third) are reversed on SYCLDevice.
648640
llvm::Metadata *AttrMDArgs[] = {
649-
llvm::ConstantAsMetadata::get(
650-
Builder.getInt32(XDimVal->getZExtValue())),
651-
llvm::ConstantAsMetadata::get(
652-
Builder.getInt32(YDimVal->getZExtValue())),
653-
llvm::ConstantAsMetadata::get(
654-
Builder.getInt32(ZDimVal->getZExtValue()))};
641+
llvm::ConstantAsMetadata::get(Builder.getInt(
642+
getLangOpts().SYCLIsDevice ? *A->getZDimVal() : *A->getXDimVal())),
643+
llvm::ConstantAsMetadata::get(Builder.getInt(*A->getYDimVal())),
644+
llvm::ConstantAsMetadata::get(Builder.getInt(
645+
getLangOpts().SYCLIsDevice ? *A->getXDimVal() : *A->getZDimVal()))};
655646
Fn->setMetadata("reqd_work_group_size",
656647
llvm::MDNode::get(Context, AttrMDArgs));
657648
}
@@ -715,7 +706,7 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD,
715706
const auto *CE = cast<ConstantExpr>(A->getValue());
716707
Optional<llvm::APSInt> ArgVal = CE->getResultAsAPSInt();
717708
llvm::Metadata *AttrMDArgs[] = {llvm::ConstantAsMetadata::get(
718-
Builder.getInt32(ArgVal->getSExtValue()))};
709+
Builder.getInt32(ArgVal->getZExtValue()))};
719710
Fn->setMetadata("num_simd_work_items",
720711
llvm::MDNode::get(Context, AttrMDArgs));
721712
}

‎clang/lib/CodeGen/TargetInfo.cpp

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8372,10 +8372,9 @@ void TCETargetCodeGenInfo::setTargetAttributes(
83728372

83738373
SmallVector<llvm::Metadata *, 5> Operands;
83748374
Operands.push_back(llvm::ConstantAsMetadata::get(F));
8375-
ASTContext &Ctx = M.getContext();
8376-
unsigned XDim = Attr->getXDimVal(Ctx)->getZExtValue();
8377-
unsigned YDim = Attr->getYDimVal(Ctx)->getZExtValue();
8378-
unsigned ZDim = Attr->getZDimVal(Ctx)->getZExtValue();
8375+
unsigned XDim = Attr->getXDimVal()->getZExtValue();
8376+
unsigned YDim = Attr->getYDimVal()->getZExtValue();
8377+
unsigned ZDim = Attr->getZDimVal()->getZExtValue();
83798378

83808379
Operands.push_back(llvm::ConstantAsMetadata::get(
83818380
llvm::Constant::getIntegerValue(M.Int32Ty, llvm::APInt(32, XDim))));
@@ -9255,9 +9254,9 @@ void AMDGPUTargetCodeGenInfo::setFunctionDeclAttributes(
92559254
Max = FlatWGS->getMax()->EvaluateKnownConstInt(Ctx).getExtValue();
92569255
}
92579256
if (ReqdWGS) {
9258-
XDim = ReqdWGS->getXDimVal(Ctx)->getZExtValue();
9259-
YDim = ReqdWGS->getYDimVal(Ctx)->getZExtValue();
9260-
ZDim = ReqdWGS->getZDimVal(Ctx)->getZExtValue();
9257+
XDim = ReqdWGS->getXDimVal()->getZExtValue();
9258+
YDim = ReqdWGS->getYDimVal()->getZExtValue();
9259+
ZDim = ReqdWGS->getZDimVal()->getZExtValue();
92619260
}
92629261
if (ReqdWGS && Min == 0 && Max == 0)
92639262
Min = Max = XDim * YDim * ZDim;

‎clang/lib/Sema/SemaDecl.cpp

Lines changed: 2 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2805,6 +2805,8 @@ static bool mergeDeclAttribute(Sema &S, NamedDecl *D,
28052805
else if (const auto *A =
28062806
dyn_cast<SYCLAddIRAttributesGlobalVariableAttr>(Attr))
28072807
NewAttr = S.MergeSYCLAddIRAttributesGlobalVariableAttr(D, *A);
2808+
else if (const auto *A = dyn_cast<ReqdWorkGroupSizeAttr>(Attr))
2809+
NewAttr = S.MergeReqdWorkGroupSizeAttr(D, *A);
28082810
else if (Attr->shouldInheritEvenIfAlreadyPresent() || !DeclHasAttr(D, Attr))
28092811
NewAttr = cast<InheritableAttr>(Attr->clone(S.Context));
28102812

@@ -3395,27 +3397,6 @@ static void adjustDeclContextForDeclaratorDecl(DeclaratorDecl *NewD,
33953397
FixSemaDC(VD->getDescribedVarTemplate());
33963398
}
33973399

3398-
template <typename AttributeType>
3399-
static void checkDimensionsAndSetDiagnostics(Sema &S, FunctionDecl *New,
3400-
FunctionDecl *Old) {
3401-
const auto *NewDeclAttr = New->getAttr<AttributeType>();
3402-
const auto *OldDeclAttr = Old->getAttr<AttributeType>();
3403-
3404-
if (!NewDeclAttr || !OldDeclAttr)
3405-
return;
3406-
3407-
ASTContext &Ctx = S.getASTContext();
3408-
if (NewDeclAttr->getXDimVal(Ctx) != OldDeclAttr->getXDimVal(Ctx) ||
3409-
NewDeclAttr->getYDimVal(Ctx) != OldDeclAttr->getYDimVal(Ctx) ||
3410-
NewDeclAttr->getZDimVal(Ctx) != OldDeclAttr->getZDimVal(Ctx)) {
3411-
S.Diag(New->getLocation(), diag::err_conflicting_sycl_function_attributes)
3412-
<< OldDeclAttr << NewDeclAttr;
3413-
S.Diag(New->getLocation(), diag::warn_duplicate_attribute) << OldDeclAttr;
3414-
S.Diag(OldDeclAttr->getLocation(), diag::note_conflicting_attribute);
3415-
S.Diag(NewDeclAttr->getLocation(), diag::note_conflicting_attribute);
3416-
}
3417-
}
3418-
34193400
/// MergeFunctionDecl - We just parsed a function 'New' from
34203401
/// declarator D which has the same name and scope as a previous
34213402
/// declaration 'Old'. Figure out how to resolve this situation,
@@ -3504,8 +3485,6 @@ bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD,
35043485
}
35053486
}
35063487

3507-
checkDimensionsAndSetDiagnostics<ReqdWorkGroupSizeAttr>(*this, New, Old);
3508-
35093488
if (const auto *ILA = New->getAttr<InternalLinkageAttr>())
35103489
if (!Old->hasAttr<InternalLinkageAttr>()) {
35113490
Diag(New->getLocation(), diag::err_attribute_missing_on_first_decl)

0 commit comments

Comments
 (0)