Skip to content

Commit 7f39b8c

Browse files
smanna12smanna12
committed
Address review comments
Signed-off-by: Soumi Manna <[email protected]>
1 parent eeaba9b commit 7f39b8c

File tree

6 files changed

+226
-64
lines changed

6 files changed

+226
-64
lines changed

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2845,6 +2845,11 @@ def ReqdWorkGroupSize : InheritableAttr {
28452845
ArrayRef<const Expr *> dimensions() const {
28462846
return {getXDim(), getYDim(), getZDim()};
28472847
}
2848+
bool isDependent() const {
2849+
return (getXDim() && getXDim()->isValueDependent()) ||
2850+
(getYDim() && getYDim()->isValueDependent()) ||
2851+
(getZDim() && getZDim()->isValueDependent());
2852+
}
28482853
Optional<llvm::APSInt> getXDimVal(ASTContext &Ctx) const {
28492854
return getXDim()->getIntegerConstantExpr(Ctx);
28502855
}

‎clang/include/clang/Basic/AttrDocs.td

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2418,9 +2418,11 @@ device kernel, the attribute is not ignored and it is propagated to the kernel.
24182418
[[intel::num_simd_work_items(N)]] void operator()() const {}
24192419
};
24202420

2421-
The ``intel::num_simd_work_items`` attribute we specify must evenly divide
2422-
the work-group size we specify for the ``intel::reqd_work_group_size`` or
2423-
``cl::reqd_work_group_size`` attribute.
2421+
If the`` intel::reqd_work_group_size`` or ``cl::reqd_work_group_size``
2422+
attribute is specified on a declaration along with a
2423+
intel::num_simd_work_items attribute, the work group size attribute
2424+
arguments must all be evenly divisible by the argument specified in
2425+
the ``intel::num_simd_work_items`` attribute.
24242426

24252427
.. code-block:: c++
24262428

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

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13071,23 +13071,25 @@ void Sema::addIntelSYCLSingleArgFunctionAttr(Decl *D,
1307113071
return;
1307213072
E = ICE.get();
1307313073
int32_t ArgInt = ArgVal.getSExtValue();
13074-
if (CI.getParsedKind() == ParsedAttr::AT_SYCLIntelNumSimdWorkItems ||
13075-
CI.getParsedKind() == ParsedAttr::AT_IntelReqdSubGroupSize) {
13074+
if (CI.getParsedKind() == ParsedAttr::AT_IntelReqdSubGroupSize) {
1307613075
if (ArgInt <= 0) {
1307713076
Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer)
13078-
<< CI.getAttrName() << /*positive*/ 0;
13077+
<< CI << /*positive*/ 0;
1307913078
return;
1308013079
}
1308113080
}
13082-
if (CI.getParsedKind() == ParsedAttr::AT_SYCLIntelMaxGlobalWorkDim) {
13081+
if (CI.getParsedKind() == ParsedAttr::AT_SYCLIntelMaxGlobalWorkDim ||
13082+
CI.getParsedKind() == ParsedAttr::AT_SYCLIntelNumSimdWorkItems) {
1308313083
if (ArgInt < 0) {
1308413084
Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer)
13085-
<< CI.getAttrName() << /*non-negative*/ 1;
13085+
<< CI << /*non-negative*/ 1;
1308613086
return;
1308713087
}
13088+
}
13089+
if (CI.getParsedKind() == ParsedAttr::AT_SYCLIntelMaxGlobalWorkDim) {
1308813090
if (ArgInt > 3) {
1308913091
Diag(E->getBeginLoc(), diag::err_attribute_argument_out_of_range)
13090-
<< CI.getAttrName() << 0 << 3 << E->getSourceRange();
13092+
<< CI << 0 << 3 << E->getSourceRange();
1309113093
return;
1309213094
}
1309313095
}
@@ -13096,7 +13098,7 @@ void Sema::addIntelSYCLSingleArgFunctionAttr(Decl *D,
1309613098
D->addAttr(::new (Context) AttrType(Context, CI, E));
1309713099
}
1309813100

13099-
static Expr *checkMaxWorkSizeAttrExpr(Sema &S, const AttributeCommonInfo &CI,
13101+
static Expr *checkWorkSizeAttrExpr(Sema &S, const AttributeCommonInfo &CI,
1310013102
Expr *E) {
1310113103
assert(E && "Attribute must have an argument.");
1310213104

@@ -13142,9 +13144,9 @@ void Sema::addIntelSYCLTripleArgFunctionAttr(Decl *D,
1314213144
!ZDimExpr->isValueDependent()) {
1314313145

1314413146
// Save ConstantExpr in semantic attribute
13145-
XDimExpr = checkMaxWorkSizeAttrExpr(*this, CI, XDimExpr);
13146-
YDimExpr = checkMaxWorkSizeAttrExpr(*this, CI, YDimExpr);
13147-
ZDimExpr = checkMaxWorkSizeAttrExpr(*this, CI, ZDimExpr);
13147+
XDimExpr = checkWorkSizeAttrExpr(*this, CI, XDimExpr);
13148+
YDimExpr = checkWorkSizeAttrExpr(*this, CI, YDimExpr);
13149+
ZDimExpr = checkWorkSizeAttrExpr(*this, CI, ZDimExpr);
1314813150

1314913151
if (!XDimExpr || !YDimExpr || !ZDimExpr)
1315013152
return;
@@ -13229,8 +13231,7 @@ FPGALoopAttrT *Sema::BuildSYCLIntelFPGALoopAttr(const AttributeCommonInfo &A,
1322913231

1323013232
if (!ArgVal) {
1323113233
Diag(E->getExprLoc(), diag::err_attribute_argument_type)
13232-
<< A.getAttrName() << AANT_ArgumentIntegerConstant
13233-
<< E->getSourceRange();
13234+
<< A << AANT_ArgumentIntegerConstant << E->getSourceRange();
1323413235
return nullptr;
1323513236
}
1323613237

@@ -13240,7 +13241,7 @@ FPGALoopAttrT *Sema::BuildSYCLIntelFPGALoopAttr(const AttributeCommonInfo &A,
1324013241
A.getParsedKind() == ParsedAttr::AT_SYCLIntelFPGALoopCoalesce) {
1324113242
if (Val <= 0) {
1324213243
Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer)
13243-
<< A.getAttrName() << /* positive */ 0;
13244+
<< A << /* positive */ 0;
1324413245
return nullptr;
1324513246
}
1324613247
} else if (A.getParsedKind() ==
@@ -13251,7 +13252,7 @@ FPGALoopAttrT *Sema::BuildSYCLIntelFPGALoopAttr(const AttributeCommonInfo &A,
1325113252
ParsedAttr::AT_SYCLIntelFPGASpeculatedIterations) {
1325213253
if (Val < 0) {
1325313254
Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer)
13254-
<< A.getAttrName() << /* non-negative */ 1;
13255+
<< A << /* non-negative */ 1;
1325513256
return nullptr;
1325613257
}
1325713258
} else {

‎clang/lib/Sema/SemaDeclAttr.cpp

Lines changed: 49 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3095,6 +3095,22 @@ static bool checkWorkGroupSizeValues(Sema &S, Decl *D, const ParsedAttr &AL) {
30953095
return Result;
30963096
}
30973097

3098+
static Expr *checkWorkSizeAttrExpr(Sema &S, const ParsedAttr &CI,
3099+
Expr *E) {
3100+
assert(E && "Attribute must have an argument.");
3101+
3102+
if (!E->isInstantiationDependent()) {
3103+
llvm::APSInt ArgVal;
3104+
ExprResult ICE = S.VerifyIntegerConstantExpression(E, &ArgVal);
3105+
3106+
if (ICE.isInvalid())
3107+
return nullptr;
3108+
3109+
E = ICE.get();
3110+
}
3111+
return E;
3112+
}
3113+
30983114
// Handles reqd_work_group_size and max_work_group_size.
30993115
template <typename WorkGroupAttr>
31003116
static void handleWorkGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) {
@@ -3132,27 +3148,28 @@ static void handleWorkGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) {
31323148

31333149
ASTContext &Ctx = S.getASTContext();
31343150

3135-
if (!XDimExpr->isValueDependent() || !YDimExpr->isValueDependent() ||
3136-
!ZDimExpr->isValueDependent()) {
3151+
if (!XDimExpr->isValueDependent() &&
3152+
!YDimExpr->isValueDependent() && !ZDimExpr->isValueDependent()) {
31373153
Optional<llvm::APSInt> XDimVal = XDimExpr->getIntegerConstantExpr(Ctx);
31383154
Optional<llvm::APSInt> YDimVal = YDimExpr->getIntegerConstantExpr(Ctx);
31393155
Optional<llvm::APSInt> ZDimVal = ZDimExpr->getIntegerConstantExpr(Ctx);
31403156

3157+
XDimExpr = checkWorkSizeAttrExpr(S, AL, XDimExpr);
3158+
YDimExpr = checkWorkSizeAttrExpr(S, AL, YDimExpr);
3159+
ZDimExpr = checkWorkSizeAttrExpr(S, AL, ZDimExpr);
3160+
3161+
if (!XDimExpr || !YDimExpr || !ZDimExpr)
3162+
return;
3163+
3164+
// Skip SEMA if we're in a template, this will be diagnosed later.
3165+
if (S.getCurLexicalContext()->isDependentContext())
3166+
return;
3167+
31413168
if (AL.getKind() == ParsedAttr::AT_ReqdWorkGroupSize) {
31423169
if (const auto *A = D->getAttr<SYCLIntelNumSimdWorkItemsAttr>()) {
31433170
int64_t NumSimdWorkItems =
31443171
A->getValue()->getIntegerConstantExpr(Ctx)->getSExtValue();
31453172

3146-
assert(NumSimdWorkItems && XDimVal &&
3147-
"Argument should be an integer constant expression");
3148-
assert(NumSimdWorkItems && YDimVal &&
3149-
"Argument should be an integer constant expression");
3150-
assert(NumSimdWorkItems && ZDimVal &&
3151-
"Argument should be an integer constant expression");
3152-
3153-
if (NumSimdWorkItems == 0)
3154-
return;
3155-
31563173
if (!(XDimVal->getZExtValue() % NumSimdWorkItems == 0 ||
31573174
YDimVal->getZExtValue() % NumSimdWorkItems == 0 ||
31583175
ZDimVal->getZExtValue() % NumSimdWorkItems == 0)) {
@@ -3169,24 +3186,17 @@ static void handleWorkGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) {
31693186
Optional<llvm::APSInt> ExistingYDimVal = ExistingAttr->getYDimVal(Ctx);
31703187
Optional<llvm::APSInt> ExistingZDimVal = ExistingAttr->getZDimVal(Ctx);
31713188

3172-
assert(XDimVal && ExistingXDimVal &&
3173-
"Argument should be an integer constant expression");
3174-
assert(YDimVal && ExistingYDimVal &&
3175-
"Argument should be an integer constant expression");
3176-
assert(ZDimVal && ExistingZDimVal &&
3177-
"Argument should be an integer constant expression");
3178-
31793189
// Compare attribute arguments value and warn for a mismatch.
31803190
if (ExistingXDimVal != XDimVal || ExistingYDimVal != YDimVal ||
31813191
ExistingZDimVal != ZDimVal) {
31823192
S.Diag(AL.getLoc(), diag::warn_duplicate_attribute) << AL;
31833193
S.Diag(ExistingAttr->getLocation(), diag::note_conflicting_attribute);
31843194
}
31853195
}
3186-
}
31873196

3188-
if (!checkWorkGroupSizeValues(S, D, AL))
3189-
return;
3197+
if (!checkWorkGroupSizeValues(S, D, AL))
3198+
return;
3199+
}
31903200

31913201
S.addIntelSYCLTripleArgFunctionAttr<WorkGroupAttr>(D, AL, XDimExpr, YDimExpr,
31923202
ZDimExpr);
@@ -3246,26 +3256,28 @@ static void handleNumSimdWorkItemsAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
32463256

32473257
S.CheckDeprecatedSYCLAttributeSpelling(AL);
32483258

3249-
if (!E->isValueDependent()) {
3259+
if (!E->isValueDependent() || !E->isInstantiationDependent()) {
32503260
ASTContext &Ctx = S.getASTContext();
3251-
Optional<llvm::APSInt> ArgVal = E->getIntegerConstantExpr(Ctx);
3261+
llvm::APSInt ArgVal;
3262+
ExprResult ICE = S.VerifyIntegerConstantExpression(E, &ArgVal);
3263+
3264+
if (ICE.isInvalid())
3265+
return;
3266+
3267+
E = ICE.get();
3268+
int64_t NumSimdWorkItems = ArgVal.getSExtValue();
3269+
3270+
if (NumSimdWorkItems == 0) {
3271+
S.Diag(E->getExprLoc(), diag::err_attribute_argument_is_zero)
3272+
<< AL << E->getSourceRange();
3273+
return;
3274+
}
32523275

32533276
if (const auto *A = D->getAttr<ReqdWorkGroupSizeAttr>()) {
32543277
Optional<llvm::APSInt> XDimVal = A->getXDimVal(Ctx);
32553278
Optional<llvm::APSInt> YDimVal = A->getYDimVal(Ctx);
32563279
Optional<llvm::APSInt> ZDimVal = A->getZDimVal(Ctx);
32573280

3258-
assert(ArgVal && XDimVal &&
3259-
"Argument should be an integer constant expression");
3260-
assert(ArgVal && YDimVal &&
3261-
"Argument should be an integer constant expression");
3262-
assert(ArgVal && ZDimVal &&
3263-
"Argument should be an integer constant expression");
3264-
3265-
int64_t NumSimdWorkItems = ArgVal->getSExtValue();
3266-
if (NumSimdWorkItems == 0)
3267-
return;
3268-
32693281
if (!(XDimVal->getZExtValue() % NumSimdWorkItems == 0 ||
32703282
YDimVal->getZExtValue() % NumSimdWorkItems == 0 ||
32713283
ZDimVal->getZExtValue() % NumSimdWorkItems == 0)) {
@@ -6035,14 +6047,14 @@ void Sema::addSYCLIntelPipeIOAttr(Decl *D, const AttributeCommonInfo &Attr,
60356047
Optional<llvm::APSInt> ArgVal = E->getIntegerConstantExpr(getASTContext());
60366048
if (!ArgVal) {
60376049
Diag(E->getExprLoc(), diag::err_attribute_argument_type)
6038-
<< Attr.getAttrName() << AANT_ArgumentIntegerConstant
6050+
<< Attr << AANT_ArgumentIntegerConstant
60396051
<< E->getSourceRange();
60406052
return;
60416053
}
60426054
int32_t ArgInt = ArgVal->getSExtValue();
60436055
if (ArgInt < 0) {
60446056
Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer)
6045-
<< Attr.getAttrName() << /*non-negative*/ 1;
6057+
<< Attr << /*non-negative*/ 1;
60466058
return;
60476059
}
60486060
}

0 commit comments

Comments
 (0)