Skip to content

[SYCL][ESIMD] Remove wrapping buffer objects into images. #2746

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.

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 4 commits into from
Nov 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions clang/include/clang/Basic/Attr.td
Original file line number Diff line number Diff line change
Expand Up @@ -1162,6 +1162,16 @@ def SYCLRegisterNum : InheritableAttr {
let PragmaAttributeSupport = 0;
}

// Used to mark ESIMD kernel pointer parameters originating from accessors.
def SYCLSimdAccessorPtr : InheritableAttr {
// No spelling, as this attribute can't be created in the source code.
let Spellings = [];
let Subjects = SubjectList<[ParmVar]>;
let LangOpts = [SYCLExplicitSIMD];
let Documentation = [SYCLSimdAccessorPtrDocs];
let PragmaAttributeSupport = 0;
}

def SYCLScope : Attr {
// No spelling, as this attribute can't be created in the source code.
let Spellings = [];
Expand Down
12 changes: 12 additions & 0 deletions clang/include/clang/Basic/AttrDocs.td
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,18 @@ def SYCLRegisterNumDocs : Documentation {
}];
}

def SYCLSimdAccessorPtrDocs : Documentation {
let Category = DocCatVariable;
let Content = [{
The ``__attribute__((esimd_acc_ptr))`` attribute is used by FE to mark ESIMD
kernel pointer parameters which correspond to the original
lambda's captured accessors. FE turns the attribute to some metadata
required by the ESIMD Back-End.
Not supposed to be used directly in the source - SYCL device compiler FE
automatically adds it for ESIMD kernels.
}];
}

def C11NoReturnDocs : Documentation {
let Category = DocCatFunction;
let Content = [{
Expand Down
6 changes: 5 additions & 1 deletion clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,8 @@ 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 +376,9 @@ class SYCLIntegrationHeader {

SourceLocation KernelLocation;

/// Whether this kernel is an ESIMD one.
bool IsESIMDKernel;

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

Expand Down
11 changes: 11 additions & 0 deletions clang/lib/CodeGen/CodeGenModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1487,6 +1487,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 @@ -1618,6 +1622,10 @@ void CodeGenModule::GenOpenCLArgMetadata(llvm::Function *Fn,
? llvm::ConstantAsMetadata::get(CGF->Builder.getInt32(
SYCLBufferLocationAttr->getLocationID()))
: llvm::ConstantAsMetadata::get(CGF->Builder.getInt32(-1)));

if (FD->hasAttr<SYCLSimdAttr>())
argESIMDAccPtrs.push_back(llvm::ConstantAsMetadata::get(
CGF->Builder.getInt1(parm->hasAttr<SYCLSimdAccessorPtrAttr>())));
}

if (LangOpts.SYCLIsDevice && !LangOpts.SYCLExplicitSIMD)
Expand All @@ -1634,6 +1642,9 @@ void CodeGenModule::GenOpenCLArgMetadata(llvm::Function *Fn,
llvm::MDNode::get(VMContext, argBaseTypeNames));
Fn->setMetadata("kernel_arg_type_qual",
llvm::MDNode::get(VMContext, argTypeQuals));
if (FD && FD->hasAttr<SYCLSimdAttr>())
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
72 changes: 59 additions & 13 deletions clang/lib/Sema/SemaSYCL.cpp
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 @@ -1714,7 +1715,11 @@ 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 @@ -1723,9 +1728,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 @@ -1819,7 +1829,10 @@ 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 @@ -1951,6 +1964,7 @@ class SyclKernelDeclCreator : public SyclKernelFieldHandler {
class SyclKernelArgsSizeChecker : public SyclKernelFieldHandler {
SourceLocation KernelLoc;
unsigned SizeOfParams = 0;
bool IsSIMD = false;

void addParam(QualType ArgTy) {
SizeOfParams +=
Expand All @@ -1960,7 +1974,9 @@ 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 @@ -1969,8 +1985,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 @@ -2044,6 +2060,19 @@ class SyclKernelArgsSizeChecker : public SyclKernelFieldHandler {
using SyclKernelFieldHandler::handleSyclHalfType;
};

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

static bool isESIMDKernelType(const CXXRecordDecl *KernelObjType) {
const CXXMethodDecl *OpParens = getOperatorParens(KernelObjType);
return (OpParens != nullptr) && OpParens->hasAttr<SYCLSimdAttr>();
}

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

const std::string &getInitMethodName() const {
bool IsSIMDKernel = isESIMDKernelType(KernelObj);
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 @@ -2367,7 +2401,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 @@ -2377,7 +2411,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 @@ -2501,7 +2535,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 @@ -2659,7 +2693,9 @@ class SyclKernelIntHeaderCreator : public SyclKernelFieldHandler {
const CXXRecordDecl *KernelObj, QualType NameType,
StringRef Name, StringRef StableName)
: SyclKernelFieldHandler(S), Header(H) {
Header.startKernel(Name, NameType, StableName, KernelObj->getLocation());
bool IsSIMDKernel = isESIMDKernelType(KernelObj);
Header.startKernel(Name, NameType, StableName, KernelObj->getLocation(),
IsSIMDKernel);
}

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

bool IsSIMDKernel = isESIMDKernelType(KernelObj);
SyclKernelArgsSizeChecker ArgsSizeChecker(*this, Args[0]->getExprLoc(),
IsSIMDKernel);

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

bool IsSIMDKernel = isESIMDKernelType(KernelObj);

// Calculate both names, since Integration headers need both.
std::string CalculatedName, StableName;
std::tie(CalculatedName, StableName) =
Expand All @@ -3095,7 +3136,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 @@ -3810,6 +3851,9 @@ 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.IsESIMDKernel
<< "; }\n";
O << "};\n";
CurStart += N;
}
Expand Down Expand Up @@ -3839,12 +3883,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().IsESIMDKernel = IsESIMDKernel;
}

void SYCLIntegrationHeader::addParamDesc(kernel_param_kind_t Kind, int Info,
Expand Down
1 change: 1 addition & 0 deletions clang/test/CodeGenSYCL/Inputs/sycl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ class accessor {
private:
void __init(__attribute__((opencl_global)) dataT *Ptr, range<dimensions> AccessRange,
range<dimensions> MemRange, id<dimensions> Offset) {}
void __init_esimd(__attribute__((opencl_global)) dataT *Ptr) {}
};

template <int dimensions, access::mode accessmode, access::target accesstarget>
Expand Down
44 changes: 44 additions & 0 deletions clang/test/CodeGenSYCL/esimd-accessor-ptr-md.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// RUN: %clang_cc1 -fsycl -fsycl-explicit-simd -fsycl-is-device \
// RUN: -internal-isystem %S/Inputs -triple spir64-unknown-unknown-sycldevice \
// RUN: -disable-llvm-passes -emit-llvm %s -o - | FileCheck %s

// This test checks
// 1) proper metadata generation for accessors used in ESIMD
// kernels:
// - Proper 'kernel_arg_accessor_ptr' metadata is generated by the FE for
// ESIMD kernels
// - Pointers originating from accessors are marked with 'buffer_t' and proper
// argument kind.
// 2) __init_esimd function is used to initialize the accessor rather than
// __init.

#include "sycl.hpp"

using namespace cl::sycl;

void test(int val) {
queue q;
q.submit([&](handler &h) {
cl::sycl::accessor<int, 1, cl::sycl::access::mode::read_write> accessorA;
cl::sycl::accessor<int, 1, cl::sycl::access::mode::read> accessorB;

h.single_task<class esimd_kernel>(
[=]() __attribute__((sycl_explicit_simd)) {
accessorA.use(val);
accessorB.use();
});
});

// --- Name
// CHECK-LABEL: define spir_kernel void @"_ZTSZZ4testiENK3$_0clERN2cl4sycl7handlerEE12esimd_kernel"(
// --- Signature
// CHECK: i32 addrspace(1)* "VCArgumentDesc"="buffer_t" "VCArgumentIOKind"="0" "VCArgumentKind"="2" %_arg_,
// CHECK: i32 "VCArgumentDesc" "VCArgumentIOKind"="0" "VCArgumentKind"="0" %_arg_1,
// CHECK: i32 addrspace(1)* "VCArgumentDesc"="buffer_t" "VCArgumentIOKind"="0" "VCArgumentKind"="2" %_arg_3)
// --- Attributes
// CHECK: {{.*}} !kernel_arg_accessor_ptr ![[ACC_PTR_ATTR:[0-9]+]] !sycl_explicit_simd !{{[0-9]+}} {{.*}}{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please check that __init_esimd is called instead of regular __init in case of ESIMD kernel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is basically proven by the fact that an accessor is translated into a single pointer arg (as in __init_esimd) rather than 4 (as in __init). I'll see if I can make direct check. Do you have suggestions, BTW?

Copy link
Contributor

@Fznamznon Fznamznon Nov 16, 2020

Choose a reason for hiding this comment

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

I don't remember exactly, but I was under impression that we don't enable early optimizations for ESIMD mode, am I right?
In case if IR is not optimized, I would just check that there are two calls to __init_esimd function, i.e. (the following LLVM IR may contain errors):

CHECK: call spir_func void @{{.*}}__init_esimd{{.*}}(%"class{{.*}}cl::sycl::accessor" addrspace(4)* {{.*}}, i32 addrspace(1)* {{.*}}) /* two times */

Or you can even check which argument of the kernel is passed like here

// CHECK: [[GEP2:%[a-zA-Z0-9_]+]] = getelementptr inbounds %"class._ZTSZ4mainE3$_0.anon", %"class._ZTSZ4mainE3$_0.anon"* [[KERNEL_OBJ]], i32 0, i32 0
.

In case if IR is optimized for ESIMD mode, we can check presence of __init_esimd in non-optimized IR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks. Will try

// --- init_esimd call is expected instead of __init:
// CHECK: call spir_func void @{{.*}}__init_esimd{{.*}}(%"{{.*}}sycl::accessor" addrspace(4)* %{{[0-9]+}}, i32 addrspace(1)* %{{[0-9]+}})
// CHECK-LABEL: }
// CHECK: ![[ACC_PTR_ATTR]] = !{i1 true, i1 false, i1 true}
}
Loading