Skip to content

[SYCL] [FPGA] Add template parameter support for work_group_size attributes #2906

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 22 commits into from
Dec 24, 2020
Merged

[SYCL] [FPGA] Add template parameter support for work_group_size attributes #2906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a42d18c
[SYCL] Add template parameter support for work_group_size attributes
smanna12 Dec 18, 2020
7f09ae5
Fix Clang format errors
smanna12 Dec 18, 2020
8c1e868
Fix Clang format errors
smanna12 Dec 18, 2020
d8790ff
Address review comments
smanna12 Dec 20, 2020
0a37b9e
Fix errors
smanna12 Dec 20, 2020
a72cc84
Fix errors
smanna12 Dec 20, 2020
05f2d08
remove extra space issues
smanna12 Dec 20, 2020
d06e05d
Address review comments
smanna12 Dec 21, 2020
e741d57
Address review comments
smanna12 Dec 22, 2020
74530d6
Merge remote-tracking branch 'my_remote/sycl' into TempSupportTripleA…
smanna12 Dec 22, 2020
730d481
update patch
smanna12 Dec 22, 2020
8359e95
Fix errors
smanna12 Dec 22, 2020
6b01ac3
Fix errors
smanna12 Dec 22, 2020
ef03d04
Fix errors
smanna12 Dec 22, 2020
8dccbfd
Address review comments
smanna12 Dec 23, 2020
e749b0e
Address review comments
smanna12 Dec 23, 2020
767ded2
add warning diagnostic for nrgative attribute values and updated tests
smanna12 Dec 23, 2020
245fa08
Accept attribute with negative value
smanna12 Dec 23, 2020
4ead54f
Fix error
smanna12 Dec 23, 2020
0ac937c
address review comments
smanna12 Dec 24, 2020
71edf5f
Remove extra space from test
smanna12 Dec 24, 2020
860b753
Reword of diagnostic message
smanna12 Dec 24, 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
23 changes: 17 additions & 6 deletions clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1287,13 +1287,18 @@ def SYCLIntelSchedulerTargetFmaxMhz : InheritableAttr {
def SYCLIntelMaxWorkGroupSize : InheritableAttr {
let Spellings = [CXX11<"intelfpga","max_work_group_size">,
CXX11<"intel","max_work_group_size">];
let Args = [UnsignedArgument<"XDim">,
UnsignedArgument<"YDim">,
UnsignedArgument<"ZDim">];
let Args = [ExprArgument<"XDim">,
ExprArgument<"YDim">,
ExprArgument<"ZDim">];
let LangOpts = [SYCLIsDevice, SYCLIsHost];
let Subjects = SubjectList<[Function], ErrorDiag>;
let Documentation = [SYCLIntelMaxWorkGroupSizeAttrDocs];
let PragmaAttributeSupport = 0;
let AdditionalMembers = [{
ArrayRef<const Expr *> dimensions() const {
return {getXDim(), getYDim(), getZDim()};
}
}];
let Documentation = [SYCLIntelMaxWorkGroupSizeAttrDocs];
}

def SYCLIntelMaxGlobalWorkDim : InheritableAttr {
Expand Down Expand Up @@ -2806,9 +2811,15 @@ def ReqdWorkGroupSize : InheritableAttr {
let Spellings = [GNU<"reqd_work_group_size">,
CXX11<"intel","reqd_work_group_size">,
CXX11<"cl","reqd_work_group_size">];
let Args = [UnsignedArgument<"XDim">, DefaultUnsignedArgument<"YDim", 1>,
DefaultUnsignedArgument<"ZDim", 1>];
let Args = [ExprArgument<"XDim">,
ExprArgument<"YDim", /*optional*/1>,
ExprArgument<"ZDim", /*optional*/1>];
let Subjects = SubjectList<[Function], ErrorDiag>;
let AdditionalMembers = [{
ArrayRef<const Expr *> dimensions() const {
return {getXDim(), getYDim(), getZDim()};
}
}];
let Documentation = [ReqdWorkGroupSizeAttrDocs];
}

Expand Down
4 changes: 3 additions & 1 deletion clang/include/clang/Basic/DiagnosticGroups.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,8 +667,10 @@ def NSReturnsMismatch : DiagGroup<"nsreturns-mismatch">;
def IndependentClassAttribute : DiagGroup<"IndependentClass-attribute">;
def UnknownAttributes : DiagGroup<"unknown-attributes">;
def IgnoredAttributes : DiagGroup<"ignored-attributes">;
def AcceptedAttributes : DiagGroup<"accepted-attributes">;
def Attributes : DiagGroup<"attributes", [UnknownAttributes,
IgnoredAttributes]>;
IgnoredAttributes,
AcceptedAttributes]>;
def UnknownSanitizers : DiagGroup<"unknown-sanitizers">;
def UnnamedTypeTemplateArgs : DiagGroup<"unnamed-type-template-args",
[CXX98CompatUnnamedTypeTemplateArgs]>;
Expand Down
4 changes: 4 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 @@ -11207,6 +11207,10 @@ def warn_attribute_spelling_deprecated : Warning<
InGroup<DeprecatedAttributes>;
def note_spelling_suggestion : Note<
"did you mean to use %0 instead?">;
def warn_attribute_requires_non_negative_integer_argument : Warning<
"the resulting value of the %0 attribute %select{first|second|third}1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay in review. I was on vacation without email access.

I think this diagnostic is too long. Can't we just say - "xyz parameter is non-negative after implicit conversion".

I don't think we need to mention "the resulting value of the %0 attribute" since the diagnostic is generated at attribute location anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Possible alternative: we could also reuse the existing diagnostic text about implicit conversion, but put it under the new attribute grouping:

def warn_attribute_requires_non_negative_integer_argument : Warning<warn_impcast_integer_sign.Text>, InGroup<AcceptedAttributes>;

One oddity from this would be that users who pass -Wno-sign-compare will still get what looks like a sign comparison warning, but that's the same behavior as with the current patch anyway.

Copy link
Contributor Author

@smanna12 smanna12 Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @AaronBallman and @elizabethandrews for the comments on improving the diagnostic message.

Does the new diagnostic print like this?
implicit conversion changes signedness: 'unsigned int' to ' int'

Example: [[intel::max_work_group_size(8, 8, -8)]]

Copy link
Contributor

Choose a reason for hiding this comment

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

The types look backwards in the diagnostic. It's changing from int to unsigned int.

Also, you should check that the diagnostic points to the attribute argument that changes sign (the -8). Does the caret appear in a place where it's obvious what the diagnostic is talking about?

Btw, a second test would be [[intel::max_work_group_size(-8, 8, -8)]] to ensure that the diagnostic triggers twice (once for each -8) and is sufficiently clear.

Copy link
Contributor Author

@smanna12 smanna12 Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also you should check that the diagnostic points to the attribute argument that changes sign (the -8).

Yes, it points to the place (-8)

Btw, a second test would be [[intel::max_work_group_size(-8, 8, -8)]] to ensure that the diagnostic triggers twice (once for each -8) and is sufficiently clear.

Yes, it triggers twice and correct place (once for each -8)

I have created #2988 for this change.

" parameter is always non-negative after implicit conversion">,
InGroup<AcceptedAttributes>;

// errors of expect.with.probability
def err_probability_not_constant_float : Error<
Expand Down
4 changes: 4 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 @@ -10081,6 +10081,10 @@ class Sema final {
template <typename AttrType>
void addIntelSYCLSingleArgFunctionAttr(Decl *D, const AttributeCommonInfo &CI,
Expr *E);
template <typename AttrType>
void addIntelSYCLTripleArgFunctionAttr(Decl *D, const AttributeCommonInfo &CI,
Expr *XDimExpr, Expr *YDimExpr,
Expr *ZDimExpr);
/// AddAlignedAttr - Adds an aligned attribute to a particular declaration.
void AddAlignedAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E,
bool IsPackExpansion);
Expand Down
49 changes: 35 additions & 14 deletions clang/lib/CodeGen/CodeGenFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,11 +620,22 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD,
}

if (const ReqdWorkGroupSizeAttr *A = FD->getAttr<ReqdWorkGroupSizeAttr>()) {
llvm::LLVMContext &Context = getLLVMContext();
Optional<llvm::APSInt> XDimVal =
A->getXDim()->getIntegerConstantExpr(FD->getASTContext());
Optional<llvm::APSInt> YDimVal =
A->getYDim()->getIntegerConstantExpr(FD->getASTContext());
Optional<llvm::APSInt> ZDimVal =
A->getZDim()->getIntegerConstantExpr(FD->getASTContext());
llvm::Metadata *AttrMDArgs[] = {
llvm::ConstantAsMetadata::get(Builder.getInt32(A->getXDim())),
llvm::ConstantAsMetadata::get(Builder.getInt32(A->getYDim())),
llvm::ConstantAsMetadata::get(Builder.getInt32(A->getZDim()))};
Fn->setMetadata("reqd_work_group_size", llvm::MDNode::get(Context, AttrMDArgs));
llvm::ConstantAsMetadata::get(
Builder.getInt32(XDimVal->getZExtValue())),
llvm::ConstantAsMetadata::get(
Builder.getInt32(YDimVal->getZExtValue())),
llvm::ConstantAsMetadata::get(
Builder.getInt32(ZDimVal->getZExtValue()))};
Fn->setMetadata("reqd_work_group_size",
llvm::MDNode::get(Context, AttrMDArgs));
}

if (const IntelReqdSubGroupSizeAttr *A =
Expand Down Expand Up @@ -670,16 +681,6 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD,
llvm::MDNode::get(Context, AttrMDArgs));
}

if (const SYCLIntelMaxWorkGroupSizeAttr *A =
FD->getAttr<SYCLIntelMaxWorkGroupSizeAttr>()) {
llvm::Metadata *AttrMDArgs[] = {
llvm::ConstantAsMetadata::get(Builder.getInt32(A->getXDim())),
llvm::ConstantAsMetadata::get(Builder.getInt32(A->getYDim())),
llvm::ConstantAsMetadata::get(Builder.getInt32(A->getZDim()))};
Fn->setMetadata("max_work_group_size",
llvm::MDNode::get(Context, AttrMDArgs));
}

if (const SYCLIntelMaxGlobalWorkDimAttr *A =
FD->getAttr<SYCLIntelMaxGlobalWorkDimAttr>()) {
llvm::LLVMContext &Context = getLLVMContext();
Expand All @@ -692,6 +693,26 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD,
llvm::MDNode::get(Context, AttrMDArgs));
}

if (const SYCLIntelMaxWorkGroupSizeAttr *A =
FD->getAttr<SYCLIntelMaxWorkGroupSizeAttr>()) {
llvm::LLVMContext &Context = getLLVMContext();
Optional<llvm::APSInt> XDimVal =
A->getXDim()->getIntegerConstantExpr(FD->getASTContext());
Optional<llvm::APSInt> YDimVal =
A->getYDim()->getIntegerConstantExpr(FD->getASTContext());
Optional<llvm::APSInt> ZDimVal =
A->getZDim()->getIntegerConstantExpr(FD->getASTContext());
llvm::Metadata *AttrMDArgs[] = {
llvm::ConstantAsMetadata::get(
Builder.getInt32(XDimVal->getZExtValue())),
llvm::ConstantAsMetadata::get(
Builder.getInt32(YDimVal->getZExtValue())),
llvm::ConstantAsMetadata::get(
Builder.getInt32(ZDimVal->getZExtValue()))};
Fn->setMetadata("max_work_group_size",
llvm::MDNode::get(Context, AttrMDArgs));
}

if (const SYCLIntelNoGlobalWorkOffsetAttr *A =
FD->getAttr<SYCLIntelNoGlobalWorkOffsetAttr>()) {
const Expr *Arg = A->getValue();
Expand Down
42 changes: 31 additions & 11 deletions clang/lib/CodeGen/TargetInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8097,16 +8097,22 @@ void TCETargetCodeGenInfo::setTargetAttributes(

SmallVector<llvm::Metadata *, 5> Operands;
Operands.push_back(llvm::ConstantAsMetadata::get(F));

Operands.push_back(
llvm::ConstantAsMetadata::get(llvm::Constant::getIntegerValue(
M.Int32Ty, llvm::APInt(32, Attr->getXDim()))));
Operands.push_back(
llvm::ConstantAsMetadata::get(llvm::Constant::getIntegerValue(
M.Int32Ty, llvm::APInt(32, Attr->getYDim()))));
Operands.push_back(
llvm::ConstantAsMetadata::get(llvm::Constant::getIntegerValue(
M.Int32Ty, llvm::APInt(32, Attr->getZDim()))));
unsigned XDim = Attr->getXDim()
->EvaluateKnownConstInt(M.getContext())
.getZExtValue();
unsigned YDim = Attr->getYDim()
->EvaluateKnownConstInt(M.getContext())
.getZExtValue();
unsigned ZDim = Attr->getZDim()
->EvaluateKnownConstInt(M.getContext())
.getZExtValue();

Operands.push_back(llvm::ConstantAsMetadata::get(
llvm::Constant::getIntegerValue(M.Int32Ty, llvm::APInt(32, XDim))));
Operands.push_back(llvm::ConstantAsMetadata::get(
llvm::Constant::getIntegerValue(M.Int32Ty, llvm::APInt(32, YDim))));
Operands.push_back(llvm::ConstantAsMetadata::get(
llvm::Constant::getIntegerValue(M.Int32Ty, llvm::APInt(32, ZDim))));

// Add a boolean constant operand for "required" (true) or "hint"
// (false) for implementing the work_group_size_hint attr later.
Expand Down Expand Up @@ -8986,6 +8992,9 @@ void AMDGPUTargetCodeGenInfo::setTargetAttributes(
if (ReqdWGS || FlatWGS) {
unsigned Min = 0;
unsigned Max = 0;
unsigned XDim = 0;
unsigned YDim = 0;
unsigned ZDim = 0;
if (FlatWGS) {
Min = FlatWGS->getMin()
->EvaluateKnownConstInt(M.getContext())
Expand All @@ -8994,8 +9003,19 @@ void AMDGPUTargetCodeGenInfo::setTargetAttributes(
->EvaluateKnownConstInt(M.getContext())
.getExtValue();
}
if (ReqdWGS) {
XDim = ReqdWGS->getXDim()
->EvaluateKnownConstInt(M.getContext())
.getZExtValue();
YDim = ReqdWGS->getYDim()
->EvaluateKnownConstInt(M.getContext())
.getZExtValue();
ZDim = ReqdWGS->getZDim()
->EvaluateKnownConstInt(M.getContext())
.getZExtValue();
}
if (ReqdWGS && Min == 0 && Max == 0)
Min = Max = ReqdWGS->getXDim() * ReqdWGS->getYDim() * ReqdWGS->getZDim();
Min = Max = XDim * YDim * ZDim;

if (Min != 0) {
assert(Min <= Max && "Min must be less than or equal Max");
Expand Down
15 changes: 12 additions & 3 deletions clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3207,9 +3207,18 @@ static void checkDimensionsAndSetDiagnostics(Sema &S, FunctionDecl *New,
if (!NewDeclAttr || !OldDeclAttr)
return;

if ((NewDeclAttr->getXDim() != OldDeclAttr->getXDim()) ||
(NewDeclAttr->getYDim() != OldDeclAttr->getYDim()) ||
(NewDeclAttr->getZDim() != OldDeclAttr->getZDim())) {
/// Returns the usigned constant integer value represented by
/// given expression.
auto getExprValue = [](const Expr *E, ASTContext &Ctx) {
return E->getIntegerConstantExpr(Ctx)->getZExtValue();
};

if ((getExprValue(NewDeclAttr->getXDim(), S.getASTContext()) !=
getExprValue(OldDeclAttr->getXDim(), S.getASTContext())) ||
(getExprValue(NewDeclAttr->getYDim(), S.getASTContext()) !=
getExprValue(OldDeclAttr->getYDim(), S.getASTContext())) ||
(getExprValue(NewDeclAttr->getZDim(), S.getASTContext()) !=
getExprValue(OldDeclAttr->getZDim(), S.getASTContext()))) {
S.Diag(New->getLocation(), diag::err_conflicting_sycl_function_attributes)
<< OldDeclAttr << NewDeclAttr;
S.Diag(New->getLocation(), diag::warn_duplicate_attribute) << OldDeclAttr;
Expand Down
Loading