Skip to content

[SYCL] Enable OpenCL diagnostics for sampler in SYCL mode #167

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 3 commits into from
Jun 12, 2019
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
40 changes: 20 additions & 20 deletions clang/lib/Sema/SemaDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6347,6 +6347,26 @@ NamedDecl *Sema::ActOnVariableDeclarator(
return nullptr;
}

if (R->isSamplerT()) {
Copy link
Contributor

@Fznamznon Fznamznon May 31, 2019

Choose a reason for hiding this comment

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

Shouldn't we guard this code under if (getLangOpts().SYCLIsDevice || getLangOpts().OpenCL) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sampler type is enabled only for OpenCL and SYCL modes, so if R->isSamplerT() returns true, we know that getLangOpts().SYCLIsDevice || getLangOpts().OpenCL is also true.
There might be some other environments which might be interested in enabling this data type, so if they what to disable this diagnostics they will have to extend the condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this code already was under if (getLangOpts().OpenCL), maybe this if could help to understand this code in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AnastasiaStulova asked for applying changes like this as removing redundant checks should speed-up compilation.

// OpenCL v1.2 s6.9.b p4:
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment looks stange. We implement some OpenCL restrictions but not only for OpenCL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I can't find any restrictions for sampler in SYCL spec. I think we can leave comment describes that SYCL re-uses OpenCL types so these restrictions applied for SYCL too but it's not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: the restriction is enabled for all languages, which allows sampler type. I don't think explicitly mentioning SYCL here is good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. So, why we are explicitly mentioning OpenCL here?

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 the place where the behavior of this type is defined.

// The sampler type cannot be used with the __local and __global address
// space qualifiers.
if (R.getAddressSpace() == LangAS::opencl_local ||
R.getAddressSpace() == LangAS::opencl_global) {
Diag(D.getIdentifierLoc(), diag::err_wrong_sampler_addressspace);
}

// OpenCL v1.2 s6.12.14.1:
// A global sampler must be declared with either the constant address
// space qualifier or with the const qualifier.
if (DC->isTranslationUnit() &&
!(R.getAddressSpace() == LangAS::opencl_constant ||
R.isConstQualified())) {
Diag(D.getIdentifierLoc(), diag::err_opencl_nonconst_global_sampler);
D.setInvalidType();
}
}

if (getLangOpts().OpenCL) {
// OpenCL v2.0 s6.9.b - Image type can only be used as a function argument.
// OpenCL v2.0 s6.13.16.1 - Pipe type can only be used as a function
Expand Down Expand Up @@ -6392,26 +6412,6 @@ NamedDecl *Sema::ActOnVariableDeclarator(
}
}

if (R->isSamplerT()) {
// OpenCL v1.2 s6.9.b p4:
// The sampler type cannot be used with the __local and __global address
// space qualifiers.
if (R.getAddressSpace() == LangAS::opencl_local ||
R.getAddressSpace() == LangAS::opencl_global) {
Diag(D.getIdentifierLoc(), diag::err_wrong_sampler_addressspace);
}

// OpenCL v1.2 s6.12.14.1:
// A global sampler must be declared with either the constant address
// space qualifier or with the const qualifier.
if (DC->isTranslationUnit() &&
!(R.getAddressSpace() == LangAS::opencl_constant ||
R.isConstQualified())) {
Diag(D.getIdentifierLoc(), diag::err_opencl_nonconst_global_sampler);
D.setInvalidType();
}
}

// OpenCL v1.2 s6.9.r:
// The event type cannot be used with the __local, __constant and __global
// address space qualifiers.
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13046,7 +13046,7 @@ ExprResult Sema::CreateBuiltinUnaryOp(SourceLocation OpLoc,
bool CanOverflow = false;

bool ConvertHalfVec = false;
if (getLangOpts().OpenCL) {
if (getLangOpts().OpenCL || getLangOpts().SYCLIsDevice) {
QualType Ty = InputExpr->getType();
// The only legal unary operation for atomics is '&'.
if ((Opc != UO_AddrOf && Ty->isAtomicType()) ||
Expand Down
10 changes: 5 additions & 5 deletions clang/lib/Sema/SemaInit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5319,9 +5319,9 @@ static bool TryOCLSamplerInitialization(Sema &S,
InitializationSequence &Sequence,
QualType DestType,
Expr *Initializer) {
if (!S.getLangOpts().OpenCL || !DestType->isSamplerT() ||
if (!DestType->isSamplerT() ||
(!Initializer->isIntegerConstantExpr(S.Context) &&
!Initializer->getType()->isSamplerT()))
!Initializer->getType()->isSamplerT()))
return false;

Sequence.AddOCLSamplerInitStep(DestType);
Expand Down Expand Up @@ -5634,6 +5634,9 @@ void InitializationSequence::InitializeFrom(Sema &S,
bool allowObjCWritebackConversion = S.getLangOpts().ObjCAutoRefCount &&
Entity.isParameterKind();

if (TryOCLSamplerInitialization(S, *this, DestType, Initializer))
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, could you please clarify: why do you need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To enable sampler initialization diagnostics in SYCL mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I didn't see why it can't work on old place. I will expand hidden code next time.
Looks like we need also move TryOCLZeroOpaqueTypeInitialization if we want enable diagnostics for event in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Event type diagnostics should be enabled separately.

return;

// We're at the end of the line for C: it's either a write-back conversion
// or it's a C assignment. There's no need to check anything else.
if (!S.getLangOpts().CPlusPlus) {
Expand All @@ -5643,9 +5646,6 @@ void InitializationSequence::InitializeFrom(Sema &S,
return;
}

if (TryOCLSamplerInitialization(S, *this, DestType, Initializer))
return;

if (TryOCLZeroOpaqueTypeInitialization(S, *this, DestType, Initializer))
return;

Expand Down
2 changes: 0 additions & 2 deletions clang/lib/Sema/SemaSYCL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,6 @@ static void buildArgTys(ASTContext &Context, CXXRecordDecl *KernelObj,
assert(InitMethod && "sampler must have __init method");

// sampler __init method has only one parameter
// void __init(__ocl_sampler_t *Sampler)
auto *FuncDecl = cast<FunctionDecl>(InitMethod);
ParmVarDecl *SamplerArg = FuncDecl->getParamDecl(0);
assert(SamplerArg && "sampler __init method must have sampler parameter");
Expand Down Expand Up @@ -912,7 +911,6 @@ static void populateIntHeader(SYCLIntegrationHeader &H, const StringRef Name,
assert(InitMethod && "sampler must have __init method");

// sampler __init method has only one argument
// void __init(__ocl_sampler_t *Sampler)
auto *FuncDecl = cast<FunctionDecl>(InitMethod);
ParmVarDecl *SamplerArg = FuncDecl->getParamDecl(0);
assert(SamplerArg && "sampler __init method must have sampler parameter");
Expand Down
8 changes: 5 additions & 3 deletions clang/lib/Sema/SemaType.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2322,7 +2322,7 @@ QualType Sema::BuildArrayType(QualType T, ArrayType::ArraySizeModifier ASM,
// OpenCL v2.0 s6.12.5 - Arrays of blocks are not supported.
// OpenCL v2.0 s6.16.13.1 - Arrays of pipe type are not supported.
// OpenCL v2.0 s6.9.b - Arrays of image/sampler type are not supported.
if (getLangOpts().OpenCL) {
if (getLangOpts().OpenCL || getLangOpts().SYCLIsDevice) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Following the logic:

Sampler type is enabled only for OpenCL and SYCL modes, so if R->isSamplerT() returns true, we know that getLangOpts().SYCLIsDevice || getLangOpts().OpenCL is also true.

Can't we remove getLangOpts checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so.
Blocks are not OpenCL specific types.

const QualType ArrType = Context.getBaseElementType(T);
if (ArrType->isBlockPointerType() || ArrType->isPipeType() ||
ArrType->isSamplerT() || ArrType->isImageType()) {
Expand Down Expand Up @@ -4409,7 +4409,7 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
// OpenCL v2.0 s6.9b - Pointer to image/sampler cannot be used.
// OpenCL v2.0 s6.13.16.1 - Pointer to pipe cannot be used.
// OpenCL v2.0 s6.12.5 - Pointers to Blocks are not allowed.
if (LangOpts.OpenCL) {
if (LangOpts.OpenCL || LangOpts.SYCLIsDevice) {
if (T->isImageType() || T->isSamplerT() || T->isPipeType() ||
T->isBlockPointerType()) {
S.Diag(D.getIdentifierLoc(), diag::err_opencl_pointer_to_type) << T;
Expand Down Expand Up @@ -4607,7 +4607,7 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
}
}

if (LangOpts.OpenCL) {
if (LangOpts.OpenCL || LangOpts.SYCLIsDevice) {
// OpenCL v2.0 s6.12.5 - A block cannot be the return value of a
// function.
if (T->isBlockPointerType() || T->isImageType() || T->isSamplerT() ||
Expand All @@ -4619,7 +4619,9 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
// OpenCL doesn't support variadic functions and blocks
// (s6.9.e and s6.12.5 OpenCL v2.0) except for printf.
// We also allow here any toolchain reserved identifiers.
// FIXME: Use deferred diagnostics engine to skip host side issues.
if (FTI.isVariadic &&
!LangOpts.SYCLIsDevice &&
!(D.getIdentifier() &&
((D.getIdentifier()->getName() == "printf" &&
(LangOpts.OpenCLCPlusPlus || LangOpts.OpenCLVersion >= 120)) ||
Expand Down
64 changes: 25 additions & 39 deletions sycl/doc/SYCL_compiler_and_runtime_design.md
Original file line number Diff line number Diff line change
Expand Up @@ -291,17 +291,28 @@ produced by OpenCL C front-end compiler.
It's a regular function, which can conflict with user code produced from C++
source.


SYCL compiler uses solution developed for OpenCL C++ compiler prototype:
SYCL compiler uses modified solution developed for OpenCL C++ compiler
prototype:

- Compiler: https://github.com/KhronosGroup/SPIR/tree/spirv-1.1
- Headers: https://github.com/KhronosGroup/libclcxx

SPIR-V types and operations that do not have LLVM equivalents are **declared**
Our solution re-uses OpenCL data types like sampler, event, image types, etc,
but we use different spelling to avoid potential conflicts with C++ code.
Spelling convention for the OpenCL types enabled in SYCL mode is:

```
__ocl_<OpenCL_type_name> // e.g. __ocl_sampler_t, __ocl_event_t
```

Operations using OpenCL types use special naming convention described in this
[document](https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/docs/SPIRVRepresentationInLLVM.rst).
This solution allows us avoid SYCL specialization in SPIR-V translator and
leverage clang infrastructure developed for OpenCL types.

SPIR-V operations that do not have LLVM equivalents are **declared**
(but not defined) in the headers and satisfy following requirements:

- the type must be pre-declared as a C++ class in `cl::__spirv` namespace
- the type must not have actual definition in C++ program
- the operation is expressed in C++ as `extern` function not throwing C++
exceptions
- the operation must not have the actual definition in C++ program
Expand All @@ -310,42 +321,17 @@ For example, the following C++ code is successfully recognized and translated
into SPIR-V operation `OpGroupAsyncCopy`:

```C++
namespace cl {
namespace __spirv {
// This class does not have definition, it is only predeclared here.
// The pointers to this class objects can be passed to or returned from
// SPIR-V built-in functions. Only in such cases the class is recognized
// as SPIR-V type OpTypeEvent.
class OpTypeEvent;

template <typename dataT>
extern OpTypeEvent *OpGroupAsyncCopy(int32_t Scope, __local dataT *Dest,
__global dataT *Src, size_t NumElements,
size_t Stride, OpTypeEvent *E) noexcept;
} // namespace __spirv
} // namespace cl

cl::__spirv::OpTypeEvent *e =
cl::__spirv::OpGroupAsyncCopy<dataT>(cl::__spirv::Scope::Workgroup,
dst, src, numElements, 1, 0);
template <typename dataT>
extern __ocl_event_t
__spirv_OpGroupAsyncCopy(int32_t Scope, __local dataT *Dest,
__global dataT *Src, size_t NumElements,
size_t Stride, __ocl_event_t E) noexcept;

__ocl_event_t e =
__spirv_OpGroupAsyncCopy(cl::__spirv::Scope::Workgroup,
dst, src, numElements, 1, E);
```

OpenCL C++ compiler uses a special module pass in clang that transforms the
names of C++ classes, globals and functions from the namespace `cl::__spirv::`
to
["SPIR-V representation in LLVM IR"](https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/docs/SPIRVRepresentationInLLVM.rst)
which is recognized by the LLVM IR to SPIR-V translator.

In the OpenCL C++ prototype project the pass is located at the directory:
`lib/CodeGen/OclCxxRewrite`. The file with the pass is:
`lib/CodeGen/OclCxxRewrite/BifNameReflower.cpp`. The other files in
`lib/CodeGen/OclCxxRewrite` are utility files implementing Itanium demangler
and other helping functionality.

That LLVM module pass has been ported from OpenCL C++ prototype to the SYCL
compiler as is. It made possible using simple declarations of C++ classes and
external functions as if they were the SPIR-V specific types and operations.

#### Some details and agreements on using SPIR-V special types and operations

The SPIR-V specific C++ enumerators and classes are declared in the file:
Expand Down