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

bader · 2019-05-24T15:36:40Z

Selectively enabled a few OpenCL diagnostics for sampler type as some
diagnostics trigger on SYCL use cases.

For instance, OpenCL kernel in SYCL mode initializes lambda captures with
the kernel argument values. This initialization code for sampler emits
errors because OpenCL disallows sampler on the right hand side of the
binary operators - these are supposed to be used only by built-in
functions.

Another potential issue is the lambda object itself -
captured sampler is a member of the lambda object and OpenCL doesn't
allow composite types with samplers. SPIR-V produced from SYCL should be
okay as lambda object can be removed by standard LLVM transformation
passes.

Fznamznon · 2019-05-24T17:31:32Z

I think if we are going to remove OpTypeSampler usages, we should also remove it from clang tests.

bader · 2019-05-26T15:07:45Z

I think if we are going to remove OpTypeSampler usages, we should also remove it from clang tests.

Thanks for reminding about this.
I've added one come commit to update the clang tests.

bader · 2019-05-28T14:13:05Z

This PR is superseded by #171.

Fznamznon · 2019-05-29T09:32:28Z

Note: in #171 diagnostics aren't enabled.

bader · 2019-05-29T09:40:37Z

Okay. I'll re-open this PR.
I'll rebase it after we merge #171.

Fznamznon · 2019-05-31T11:38:55Z

clang/lib/Sema/SemaDecl.cpp

@@ -6347,6 +6347,26 @@ NamedDecl *Sema::ActOnVariableDeclarator(
    return nullptr;
  }

+  if (R->isSamplerT()) {


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

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.

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

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

Fznamznon · 2019-05-31T11:53:02Z

clang/lib/Sema/SemaDecl.cpp

@@ -6347,6 +6347,26 @@ NamedDecl *Sema::ActOnVariableDeclarator(
    return nullptr;
  }

+  if (R->isSamplerT()) {
+    // OpenCL v1.2 s6.9.b p4:


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

Yes. Any suggestions?

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.

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

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

This is the place where the behavior of this type is defined.

Fznamznon · 2019-05-31T11:55:22Z

clang/lib/Sema/SemaInit.cpp

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

+  if (TryOCLSamplerInitialization(S, *this, DestType, Initializer))


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

To enable sampler initialization diagnostics in SYCL mode.

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.

Event type diagnostics should be enabled separately.

romanovvlad · 2019-06-02T14:02:08Z

clang/lib/Sema/SemaType.cpp

@@ -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) {


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?

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

clang/lib/Sema/SemaSYCL.cpp

Removed comments are redundant and complicate maintenance. Signed-off-by: Alexey Bader <alexey.bader@intel.com>

Signed-off-by: Alexey Bader <alexey.bader@intel.com>

Selectively enabled a few OpenCL diagnostics for sampler type as some diagnostics trigger on SYCL use cases. For instance, OpenCL kernel in SYCL mode initializes lambda captures with the kernel argument values. This initialization code for sampler emits errors because OpenCL disallows sampler on the right hand side of the binary operators - these are supposed to be used only by built-in functions. Another potential issue is the lambda object itself - captured sampler is a member of the lambda object and OpenCL doesn't allow composite types with samplers. SPIR-V produced from SYCL should be okay as lambda object can be removed by standard LLVM transformation passes. Signed-off-by: Alexey Bader <alexey.bader@intel.com>

This was referenced May 24, 2019

[SYCL] Re-use OpenCL sampler in SYCL mode bader/llvm#1

Closed

[SYCL] How OpenCL device mode is being reused for SYCL #59

Open

bader force-pushed the sampler-experiment branch from a36ebb2 to 5a3d2d1 Compare May 26, 2019 15:05

bader mentioned this pull request May 27, 2019

[SYCL] Remove re-flower pass #152

Closed

bader changed the title ~~[WIP][SYCL] Switch SPIR-V types to OpenCL types~~ [SYCL] Switch SPIR-V types to OpenCL types May 27, 2019

bader closed this May 28, 2019

bader reopened this May 29, 2019

bader changed the title ~~[SYCL] Switch SPIR-V types to OpenCL types~~ [WIP][SYCL] Switch SPIR-V types to OpenCL types May 29, 2019

bader force-pushed the sampler-experiment branch from 5a3d2d1 to 3a90f83 Compare May 31, 2019 10:57

bader changed the title ~~[WIP][SYCL] Switch SPIR-V types to OpenCL types~~ [SYCL] Enable OpenCL diagnostics for sampler in SYCL mode May 31, 2019

bader requested a review from Fznamznon May 31, 2019 10:59

Fznamznon reviewed May 31, 2019

View reviewed changes

romanovvlad reviewed Jun 2, 2019

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

bader added 3 commits June 7, 2019 16:15

[SYCL] Update documentation with OpenCL types re-use proposal

809cdc3

Signed-off-by: Alexey Bader <alexey.bader@intel.com>

bader force-pushed the sampler-experiment branch from 3a90f83 to ec5e1b3 Compare June 7, 2019 16:48

Fznamznon approved these changes Jun 11, 2019

View reviewed changes

bader merged commit eb735ee into intel:sycl Jun 12, 2019

bader deleted the sampler-experiment branch June 21, 2019 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bader commented May 24, 2019 •

edited

Loading

Fznamznon commented May 24, 2019

bader commented May 26, 2019

bader commented May 28, 2019

Fznamznon commented May 29, 2019

bader commented May 29, 2019

Fznamznon May 31, 2019 •

edited

Loading

bader May 31, 2019

Fznamznon May 31, 2019

bader Jun 7, 2019

Fznamznon May 31, 2019

bader May 31, 2019

Fznamznon May 31, 2019

bader Jun 7, 2019

Fznamznon Jun 10, 2019

bader Jun 11, 2019

Fznamznon May 31, 2019

bader May 31, 2019

Fznamznon May 31, 2019

bader Jun 7, 2019

romanovvlad Jun 2, 2019

bader Jun 7, 2019

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

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

Conversation

bader commented May 24, 2019 • edited Loading

Fznamznon commented May 24, 2019

bader commented May 26, 2019

bader commented May 28, 2019

Fznamznon commented May 29, 2019

bader commented May 29, 2019

Fznamznon May 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bader commented May 24, 2019 •

edited

Loading

Fznamznon May 31, 2019 •

edited

Loading