Skip to content

[SYCL][ESIMD] Refactor ESIMD kernel support in FE. #2747

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

Closed
kbobrovs wants to merge 8 commits into from
Closed

[SYCL][ESIMD] Refactor ESIMD kernel support in FE. #2747

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8e521ff
[SYCL][ESIMD] Add 'esimd_acc_ptr' parameter attribute to FE.
kbobrovs Nov 3, 2020
d4f5813
[SYCL][ESIMD] Refactor ESIMD kernel support in FE.
kbobrovs Nov 3, 2020
e5ab3ef
Update clang/lib/Sema/SemaSYCL.cpp
kbobrovs Nov 10, 2020
f9c7ea2
[FE] Don't generate kernel_arg_accessor_ptr MD for non-ESIMD kernels.
kbobrovs Nov 11, 2020
b442079
[FE] Address review comments
kbobrovs Nov 12, 2020
95053d4
[FE] Apply clang-format
kbobrovs Nov 12, 2020
bb9c41e
[FE] Address review comment - use hasAttr vs getAttr != nullptr
kbobrovs Nov 12, 2020
d3b1493
[FE] Fix crash in CodeGenModule.cpp
kbobrovs Nov 12, 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
5 changes: 4 additions & 1 deletion clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ class SYCLIntegrationHeader {
/// Signals that subsequent parameter descriptor additions will go to
/// the kernel with given name. Starts new kernel invocation descriptor.
void startKernel(StringRef KernelName, QualType KernelNameType,
StringRef KernelStableName, SourceLocation Loc);
StringRef KernelStableName, SourceLocation Loc, bool IsESIMD);

/// Adds a kernel parameter descriptor to current kernel invocation
/// descriptor.
Expand Down Expand Up @@ -375,6 +375,9 @@ class SYCLIntegrationHeader {

SourceLocation KernelLocation;

/// Whether this kernel is an ESIMD one.
bool IsESIMD;
Copy link
Contributor

@Fznamznon Fznamznon Nov 10, 2020

Choose a reason for hiding this comment

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

How about

Suggested change
bool IsESIMD;
bool IsESIMDKernel;

To make it clear that this flag is about kernel and not language options, for example.

Copy link
Contributor Author

@kbobrovs kbobrovs Nov 10, 2020

Choose a reason for hiding this comment

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

ok


/// Descriptor of kernel actual parameters.
SmallVector<KernelParamDesc, 8> Params;

Expand Down
10 changes: 10 additions & 0 deletions clang/lib/CodeGen/CodeGenModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1439,6 +1439,10 @@ void CodeGenModule::GenOpenCLArgMetadata(llvm::Function *Fn,
// MDNode for the intel_buffer_location attribute.
SmallVector<llvm::Metadata *, 8> argSYCLBufferLocationAttr;

// MDNode for listing ESIMD kernel pointer arguments originating from
// accessors
SmallVector<llvm::Metadata *, 8> argESIMDAccPtrs;

if (FD && CGF)
for (unsigned i = 0, e = FD->getNumParams(); i != e; ++i) {
const ParmVarDecl *parm = FD->getParamDecl(i);
Expand Down Expand Up @@ -1570,6 +1574,10 @@ void CodeGenModule::GenOpenCLArgMetadata(llvm::Function *Fn,
? llvm::ConstantAsMetadata::get(CGF->Builder.getInt32(
SYCLBufferLocationAttr->getLocationID()))
: llvm::ConstantAsMetadata::get(CGF->Builder.getInt32(-1)));

argESIMDAccPtrs.push_back(
llvm::ConstantAsMetadata::get(CGF->Builder.getInt1(
parm->getAttr<SYCLSimdAccessorPtrAttr>() != nullptr)));
Copy link
Contributor

@Fznamznon Fznamznon Nov 10, 2020

Choose a reason for hiding this comment

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

I think it will be a bit more optimal.

Suggested change
parm->getAttr<SYCLSimdAccessorPtrAttr>() != nullptr)));
parm->hasAttr<SYCLSimdAccessorPtrAttr>())));

Copy link
Contributor Author

@kbobrovs kbobrovs Nov 10, 2020

Choose a reason for hiding this comment

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

not sure. I want 0 or 1 is a clean input for getInt1. Non-null parm->getAttr() is neither

Copy link
Contributor

@Fznamznon Fznamznon Nov 10, 2020

Choose a reason for hiding this comment

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

Sorry, I didn't get your statement. I thought that parm->getAttr<SYCLSimdAccessorPtrAttr>() != nullptr actually gives bool which is 0 or 1 as well as hasAttr function which also returns bool (https://clang.llvm.org/doxygen/classclang_1_1Decl.html#ae7a63b99398864d86f53afd8a03c359b).

Copy link
Contributor Author

@kbobrovs kbobrovs Nov 12, 2020

Choose a reason for hiding this comment

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

Sorry, you are right. hasAttr is OK - will change

}

if (LangOpts.SYCLIsDevice && !LangOpts.SYCLExplicitSIMD)
Expand All @@ -1586,6 +1594,8 @@ void CodeGenModule::GenOpenCLArgMetadata(llvm::Function *Fn,
llvm::MDNode::get(VMContext, argBaseTypeNames));
Fn->setMetadata("kernel_arg_type_qual",
llvm::MDNode::get(VMContext, argTypeQuals));
Fn->setMetadata("kernel_arg_accessor_ptr",
llvm::MDNode::get(VMContext, argESIMDAccPtrs));
if (getCodeGenOpts().EmitOpenCLArgMetadata)
Fn->setMetadata("kernel_arg_name",
llvm::MDNode::get(VMContext, argNames));
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8209,6 +8209,9 @@ static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
case ParsedAttr::AT_SYCLRegisterNum:
handleSYCLRegisterNumAttr(S, D, AL);
break;
case ParsedAttr::AT_SYCLSimdAccessorPtr:
handleSimpleAttribute<SYCLSimdAccessorPtrAttr>(S, D, AL);
break;
Copy link
Contributor

@Fznamznon Fznamznon Nov 10, 2020

Choose a reason for hiding this comment

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

I guess we won't need this part if we remove spelling.

Copy link
Contributor Author

@kbobrovs kbobrovs Nov 10, 2020

Choose a reason for hiding this comment

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

ok

case ParsedAttr::AT_Format:
handleFormatAttr(S, D, AL);
break;
Expand Down
67 changes: 54 additions & 13 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ enum KernelInvocationKind {
};

const static std::string InitMethodName = "__init";
const static std::string InitESIMDMethodName = "__init_esimd";
const static std::string FinalizeMethodName = "__finalize";
constexpr unsigned MaxKernelArgsSize = 2048;

Expand Down Expand Up @@ -1700,7 +1701,8 @@ class SyclKernelDeclCreator : public SyclKernelFieldHandler {
bool isAccessorType = false) {
const auto *RecordDecl = FieldTy->getAsCXXRecordDecl();
assert(RecordDecl && "The accessor/sampler must be a RecordDecl");
CXXMethodDecl *InitMethod = getMethodByName(RecordDecl, InitMethodName);
const std::string &MethodName = KernelDecl->hasAttr<SYCLSimdAttr>() && isAccessorType ? InitESIMDMethodName : InitMethodName;
CXXMethodDecl *InitMethod = getMethodByName(RecordDecl, MethodName);
assert(InitMethod && "The accessor/sampler must have the __init method");

// Don't do -1 here because we count on this to be the first parameter added
Expand All @@ -1709,9 +1711,14 @@ class SyclKernelDeclCreator : public SyclKernelFieldHandler {
for (const ParmVarDecl *Param : InitMethod->parameters()) {
QualType ParamTy = Param->getType();
addParam(FD, ParamTy.getCanonicalType());
if (ParamTy.getTypePtr()->isPointerType() && isAccessorType)
if (ParamTy.getTypePtr()->isPointerType() && isAccessorType) {
handleAccessorPropertyList(Params.back(), RecordDecl,
FD->getLocation());
if (KernelDecl->hasAttr<SYCLSimdAttr>())
// In ESIMD kernels accessor's pointer argument needs to be marked
Params.back()->addAttr(
SYCLSimdAccessorPtrAttr::CreateImplicit(SemaRef.getASTContext()));
}
}
LastParamIndex = ParamIndex;
return true;
Expand Down Expand Up @@ -1805,7 +1812,8 @@ class SyclKernelDeclCreator : public SyclKernelFieldHandler {
QualType FieldTy) final {
const auto *RecordDecl = FieldTy->getAsCXXRecordDecl();
assert(RecordDecl && "The accessor/sampler must be a RecordDecl");
CXXMethodDecl *InitMethod = getMethodByName(RecordDecl, InitMethodName);
const std::string MethodName = KernelDecl->hasAttr<SYCLSimdAttr>() ? InitESIMDMethodName : InitMethodName;
CXXMethodDecl *InitMethod = getMethodByName(RecordDecl, MethodName);
assert(InitMethod && "The accessor/sampler must have the __init method");

// Don't do -1 here because we count on this to be the first parameter added
Expand Down Expand Up @@ -1937,6 +1945,7 @@ class SyclKernelDeclCreator : public SyclKernelFieldHandler {
class SyclKernelArgsSizeChecker : public SyclKernelFieldHandler {
SourceLocation KernelLoc;
unsigned SizeOfParams = 0;
bool IsSIMD = false;

void addParam(QualType ArgTy) {
SizeOfParams +=
Expand All @@ -1946,7 +1955,8 @@ class SyclKernelArgsSizeChecker : public SyclKernelFieldHandler {
bool handleSpecialType(QualType FieldTy) {
const CXXRecordDecl *RecordDecl = FieldTy->getAsCXXRecordDecl();
assert(RecordDecl && "The accessor/sampler must be a RecordDecl");
CXXMethodDecl *InitMethod = getMethodByName(RecordDecl, InitMethodName);
const std::string &MethodName = IsSIMD ? InitESIMDMethodName : InitMethodName;
CXXMethodDecl *InitMethod = getMethodByName(RecordDecl, MethodName);
assert(InitMethod && "The accessor/sampler must have the __init method");
for (const ParmVarDecl *Param : InitMethod->parameters())
addParam(Param->getType());
Expand All @@ -1955,8 +1965,8 @@ class SyclKernelArgsSizeChecker : public SyclKernelFieldHandler {

public:
static constexpr const bool VisitInsideSimpleContainers = false;
SyclKernelArgsSizeChecker(Sema &S, SourceLocation Loc)
: SyclKernelFieldHandler(S), KernelLoc(Loc) {}
SyclKernelArgsSizeChecker(Sema &S, SourceLocation Loc, bool IsSIMD)
: SyclKernelFieldHandler(S), KernelLoc(Loc), IsSIMD(IsSIMD) {}

~SyclKernelArgsSizeChecker() {
if (SizeOfParams > MaxKernelArgsSize)
Expand Down Expand Up @@ -2030,6 +2040,15 @@ class SyclKernelArgsSizeChecker : public SyclKernelFieldHandler {
using SyclKernelFieldHandler::handleSyclHalfType;
};

static const CXXMethodDecl *getOperatorParens(const CXXRecordDecl *Rec) {
for (const auto *D : Rec->decls()) {
if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D))
if (MD->getOverloadedOperator() == OO_Call)
return MD;
}
return nullptr;
}

class SyclKernelBodyCreator : public SyclKernelFieldHandler {
SyclKernelDeclCreator &DeclCreator;
llvm::SmallVector<Stmt *, 16> BodyStmts;
Expand Down Expand Up @@ -2345,6 +2364,13 @@ class SyclKernelBodyCreator : public SyclKernelFieldHandler {
return VD;
}

const std::string& getInitMethodName() const {
const CXXMethodDecl *OpParens = getOperatorParens(KernelObj);
bool IsSIMDKernel =
(OpParens != nullptr) && OpParens->hasAttr<SYCLSimdAttr>();
return IsSIMDKernel ? InitESIMDMethodName : InitMethodName;
}

// Default inits the type, then calls the init-method in the body.
bool handleSpecialType(FieldDecl *FD, QualType Ty) {
addFieldInit(FD, Ty, None,
Expand All @@ -2353,7 +2379,7 @@ class SyclKernelBodyCreator : public SyclKernelFieldHandler {
addFieldMemberExpr(FD, Ty);

const auto *RecordDecl = Ty->getAsCXXRecordDecl();
createSpecialMethodCall(RecordDecl, InitMethodName, BodyStmts);
createSpecialMethodCall(RecordDecl, getInitMethodName(), BodyStmts);

removeFieldMemberExpr(FD, Ty);

Expand All @@ -2363,7 +2389,7 @@ class SyclKernelBodyCreator : public SyclKernelFieldHandler {
bool handleSpecialType(const CXXBaseSpecifier &BS, QualType Ty) {
const auto *RecordDecl = Ty->getAsCXXRecordDecl();
addBaseInit(BS, Ty, InitializationKind::CreateDefault(KernelCallerSrcLoc));
createSpecialMethodCall(RecordDecl, InitMethodName, BodyStmts);
createSpecialMethodCall(RecordDecl, getInitMethodName(), BodyStmts);
return true;
}

Expand Down Expand Up @@ -2487,7 +2513,7 @@ class SyclKernelBodyCreator : public SyclKernelFieldHandler {
// calls, so add them here instead.
const auto *StreamDecl = Ty->getAsCXXRecordDecl();

createSpecialMethodCall(StreamDecl, InitMethodName, BodyStmts);
createSpecialMethodCall(StreamDecl, getInitMethodName(), BodyStmts);
createSpecialMethodCall(StreamDecl, FinalizeMethodName, FinalizeStmts);

removeFieldMemberExpr(FD, Ty);
Expand Down Expand Up @@ -2645,7 +2671,11 @@ class SyclKernelIntHeaderCreator : public SyclKernelFieldHandler {
const CXXRecordDecl *KernelObj, QualType NameType,
StringRef Name, StringRef StableName)
: SyclKernelFieldHandler(S), Header(H) {
Header.startKernel(Name, NameType, StableName, KernelObj->getLocation());
const CXXMethodDecl *OpParens = getOperatorParens(KernelObj);
bool IsSIMDKernel =
(OpParens != nullptr) && OpParens->hasAttr<SYCLSimdAttr>();

Header.startKernel(Name, NameType, StableName, KernelObj->getLocation(), IsSIMDKernel);
}

bool handleSyclAccessorType(const CXXRecordDecl *RD,
Expand Down Expand Up @@ -3012,7 +3042,10 @@ void Sema::CheckSYCLKernelCall(FunctionDecl *KernelFunc, SourceRange CallLoc,
SyclKernelDecompMarker DecompMarker(*this);
SyclKernelFieldChecker FieldChecker(*this);
SyclKernelUnionChecker UnionChecker(*this);
SyclKernelArgsSizeChecker ArgsSizeChecker(*this, Args[0]->getExprLoc());

const CXXMethodDecl *OpParens = getOperatorParens(KernelObj);
bool IsSIMD = (OpParens != nullptr) && OpParens->hasAttr<SYCLSimdAttr>();
Copy link
Contributor

@DenisBakhvalov DenisBakhvalov Nov 10, 2020

Choose a reason for hiding this comment

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

nit: Those two lines are repeated multiple times. Maybe create a helper function?

Copy link
Contributor Author

@kbobrovs kbobrovs Nov 10, 2020

Choose a reason for hiding this comment

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

ok

SyclKernelArgsSizeChecker ArgsSizeChecker(*this, Args[0]->getExprLoc(), IsSIMD);

KernelObjVisitor Visitor{*this};
SYCLKernelNameTypeVisitor KernelNameTypeVisitor(*this, Args[0]->getExprLoc(),
Expand Down Expand Up @@ -3073,6 +3106,10 @@ void Sema::ConstructOpenCLKernel(FunctionDecl *KernelCallerFunc,
if (KernelObj->isInvalidDecl())
return;

const CXXMethodDecl *OpParens = getOperatorParens(KernelObj);
bool IsSIMDKernel =
(OpParens != nullptr) && OpParens->hasAttr<SYCLSimdAttr>();

// Calculate both names, since Integration headers need both.
std::string CalculatedName, StableName;
std::tie(CalculatedName, StableName) =
Expand All @@ -3081,7 +3118,7 @@ void Sema::ConstructOpenCLKernel(FunctionDecl *KernelCallerFunc,
: CalculatedName);
SyclKernelDeclCreator kernel_decl(*this, KernelName, KernelObj->getLocation(),
KernelCallerFunc->isInlined(),
KernelCallerFunc->hasAttr<SYCLSimdAttr>());
IsSIMDKernel);
SyclKernelBodyCreator kernel_body(*this, kernel_decl, KernelObj,
KernelCallerFunc);
SyclKernelIntHeaderCreator int_header(
Expand Down Expand Up @@ -3795,6 +3832,8 @@ void SYCLIntegrationHeader::emit(raw_ostream &O) {
O << "getParamDesc(unsigned i) {\n";
O << " return kernel_signatures[i+" << CurStart << "];\n";
O << " }\n";
O << " __SYCL_DLL_LOCAL\n";
O << " static constexpr bool isESIMD() { return " << K.IsESIMD << "; }\n";
O << "};\n";
CurStart += N;
}
Expand Down Expand Up @@ -3824,12 +3863,14 @@ bool SYCLIntegrationHeader::emit(const StringRef &IntHeaderName) {
void SYCLIntegrationHeader::startKernel(StringRef KernelName,
QualType KernelNameType,
StringRef KernelStableName,
SourceLocation KernelLocation) {
SourceLocation KernelLocation,
bool IsESIMDKernel) {
KernelDescs.resize(KernelDescs.size() + 1);
KernelDescs.back().Name = std::string(KernelName);
KernelDescs.back().NameType = KernelNameType;
KernelDescs.back().StableName = std::string(KernelStableName);
KernelDescs.back().KernelLocation = KernelLocation;
KernelDescs.back().IsESIMD = IsESIMDKernel;
}

void SYCLIntegrationHeader::addParamDesc(kernel_param_kind_t Kind, int Info,
Expand Down