[SYCL] moving type checks to later in Semantic Analysis lifecycle #1465

cperkinsintel · 2020-04-02T18:39:26Z

SPIR_V won't support certain types ( quad type, __int128, zero length arrays and other). So we check any code that is running on the kernel for those types and emit an error if they are encountered.

Presently, these checks are done in ConvertDeclSpecToType in SemaType.cpp, but this is too early in the Sema lifecycle. It can catch only the most straightforward type declarations. We need to also catch types that use auto declarations, typedef, templating and more. To do that, the type checking must occur a little later in the Semantic Analysis lifecycle.

Here I am calling the checks from CheckCompleteVariableDeclaration in SemaDecl.cpp. This is called just after the parsing is finished and seems to work well. Also, these checks now avoid the problems we encountered with deferred diagnostics getting confused by templated functions. So, no further change is needed to the deferred diagnostic system (as far as these checks are concerned).

Expanded the testing as well.

Signed-off-by: Chris Perkins chris.perkins@intel.com

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-04-02T19:34:08Z

clang/include/clang/Sema/Sema.h

@@ -12303,6 +12303,7 @@ class Sema final {
  };

  bool isKnownGoodSYCLDecl(const Decl *D);
+  void CheckVarDeclOKIfInKernel(VarDecl *var);


You likely cannot use this name, as this would imply it also affects CUDA, OMP, and OpenCL.

Should I put "SYCL" into its name?

I have rename this to be checkSYCLVarDeclIfInKernel and the variable param is capitalized. Let me know if there is anything else.

clang/lib/Sema/SemaSYCL.cpp

clang/lib/Sema/SemaType.cpp

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-04-02T19:40:25Z

clang/test/SemaSYCL/deferred-diagnostics-emit.cpp

@@ -2,13 +2,18 @@
 //
 // Ensure that the SYCL diagnostics that are typically deferred are correctly emitted.

+namespace std {


Do you validate variable templates anywhere? How about alias templates?

I put those over in sycl-restrict.cpp, along with checks for auto, typedef, and some false postives. Let me know if you have any cases to add.

In this file, we're just exercising that the deferred diagnostics are working when the kernel lambda is itself templated.

I don't see any variable template or alias template examples over there. Can you point them out please?

I misunderstood you the first time. I have added alias templates and C++14 variable templates to sycl-restrict.cpp (starting at line 116) as both cases that should be detected and possible false positive cases that should not be flagged.

If you see anything else that we should check, let me know.

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

…he Semantic Analysis lifecycle. Signed-off-by: Chris Perkins <chris.perkins@intel.com>

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

erichkeane · 2020-04-03T01:03:26Z

clang/include/clang/Sema/Sema.h

@@ -12455,6 +12455,7 @@ class Sema final {
  };

  bool isKnownGoodSYCLDecl(const Decl *D);
+  void checkSYCLVarDeclIfInKernel(VarDecl *Var);


Still kind of hate the name, seems clumsy, but I don't have a better suggestion at the moment, I'll think about it.

Suggested change

void checkSYCLVarDeclIfInKernel(VarDecl *Var);

void checkSYCLVarDeclInSYCLKernel(VarDecl *Var);

since SYCL is about the kernel, not the variable?

SYCL is not only about kernels. Let's call it "device code".

Suggested change

void checkSYCLVarDeclIfInKernel(VarDecl *Var);

void diagnoseVarDeclIfSYCLDeviceCode(VarDecl *Var);

How about 'checkSYCLDeviceVarDecl'. "check" is the common word for a function that diagnoses. The word "if" is incorrect, since this isn't checking whether it is in a SYCL device.

The above also is more consistent in the cases we've added where we recursively check.

Hmm, okay. Let it be checkSYCLDeviceVarDecl.

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-04-03T01:05:19Z

clang/lib/Sema/SemaSYCL.cpp

+  if (isZeroSizedArray(Ty))
+    SYCLDiagIfDeviceCode(Loc.getBegin(), diag::err_typecheck_zero_array_size);
+
+  // TODO: check type of accessor


BTW I have a feeling that it is not a proper place for accessor type checking. Just because accessors always declared in host code and captured to the device code. We can instead:

Check type of buffer/accessor data using static assertions in SYCL headers.

Check type of accessors while we iterate over kernel object captures in SemaSYCL (where check for trivially destructible/constructible kernel arguments here).

I personally prefer the way with SYCL headers, since SemaSYCL extremely big, complicated and ugly.

I'll remove the comment. This is the next thing I am planning to undertake, but not as part of this PR.

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-04-03T01:06:46Z

clang/test/SemaSYCL/deferred-diagnostics-emit.cpp

@@ -2,13 +2,18 @@
 //
 // Ensure that the SYCL diagnostics that are typically deferred are correctly emitted.

+namespace std {


I don't see any variable template or alias template examples over there. Can you point them out please?

clang/lib/Sema/SemaSYCL.cpp

Fznamznon

Okay, as far as I understand this helps only with declarations made inside the device code. This won't help with cases like function parameters and captures from host to device. But it's much more better than what we had before, thanks for working on this!

clang/lib/Sema/SemaSYCL.cpp

Fznamznon · 2020-04-03T10:30:39Z

clang/include/clang/Sema/Sema.h

@@ -12455,6 +12455,7 @@ class Sema final {
  };

  bool isKnownGoodSYCLDecl(const Decl *D);
+  void checkSYCLVarDeclIfInKernel(VarDecl *Var);


SYCL is not only about kernels. Let's call it "device code".

Suggested change

void checkSYCLVarDeclIfInKernel(VarDecl *Var);

void diagnoseVarDeclIfSYCLDeviceCode(VarDecl *Var);

clang/lib/Sema/SemaSYCL.cpp

Fznamznon · 2020-04-03T10:38:58Z

clang/lib/Sema/SemaSYCL.cpp

+  if (isZeroSizedArray(Ty))
+    SYCLDiagIfDeviceCode(Loc.getBegin(), diag::err_typecheck_zero_array_size);
+
+  // TODO: check type of accessor


BTW I have a feeling that it is not a proper place for accessor type checking. Just because accessors always declared in host code and captured to the device code. We can instead:

Check type of buffer/accessor data using static assertions in SYCL headers.

Check type of accessors while we iterate over kernel object captures in SemaSYCL (where check for trivially destructible/constructible kernel arguments here).

I personally prefer the way with SYCL headers, since SemaSYCL extremely big, complicated and ugly.

Fznamznon · 2020-04-03T10:43:03Z

clang/test/SemaSYCL/sycl-restrict.cpp


-  __float128 A; // expected-error {{__float128 is not supported on this target}}
+  // ======= Float128 Not Allowed in Kernel ==========


Does it diagnose if some struct has a field with unsupported type and this variable with this struct type is declared inside device code?

I will check it and see.
Right now, the only SYCL code the type checks struct members is the recursive type check code that is in CheckSYCLType over in SemaSYCL.cpp, the one called by the AST visitor.

Hmm - so struct members are problematic. They are overlooked.

I can recursively check them, that works well enough. It correctly issues an error if the violating struct is used in a kernel context, when that error is issued it is on the struct declaration itself. Great. But it's confusing, because declaring the struct isn't really a problem, using it in a kernel context is.

Right now, the checks we have in the AST Visitor, since they don't use the deferred diagnostics, correctly recurse struc members and output an error on its declaration AND they also issue the diag::note_sycl_used_here to guide the user back to the usage that triggers it. But this same approach isn't practical with deferred diagnostics.

Let me investigate some more. I can think of a handful of approaches.

OK - I brought some of the recursive struct code out of CheckSYCLType. Tidied it up and got the notes to emit as a follow on deferred diagnostic. Struct member are working now.

…king, and expanded testing Signed-off-by: Chris Perkins <chris.perkins@intel.com>

cperkinsintel · 2020-04-06T15:36:22Z

I've made all the changes requested, including recursively checking struct members and testing for alias and variable templates. Let me know if there is anything else.

erichkeane · 2020-04-06T15:49:03Z

clang/include/clang/Sema/Sema.h

@@ -12455,6 +12455,7 @@ class Sema final {
  };

  bool isKnownGoodSYCLDecl(const Decl *D);
+  void checkSYCLVarDeclIfInKernel(VarDecl *Var);


How about 'checkSYCLDeviceVarDecl'. "check" is the common word for a function that diagnoses. The word "if" is incorrect, since this isn't checking whether it is in a SYCL device.

The above also is more consistent in the cases we've added where we recursively check.

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

Fznamznon · 2020-04-07T07:14:20Z

clang/lib/Sema/SemaSYCL.cpp

+  // Not all variable types are supported inside SYCL kernels,
+  // for example the quad type __float128 will cause the resulting
+  // SPIR-V to not link.


__float128 will cause the resulting SPIR-V to not link.

What exactly do you mean here? AFAIK we don't link SPIR-V files.

I am not an expert on the pipeline, especially in regards to how SPIR-V is handled. In the simplest case, without these type checks, if you have kernel code that contains an unsupported type (such as __int128) , you can pass the -c flag to the compiler to simply output the object files, and everything is fine. But when you next go to link those files into your executable, you'll get an error ( InvalidBitWidth: Invalid bit width in input: 128 etc) . IIRC, those are emitted by SpirVWriter.cpp
So it may not be linking, per se. But it occurs in what we normally think of as the link phase.

Work with @Fznamznon to make sure that this comment means what you want it to.

Hmm okay, such errors actually happen during LLVM IR -> SPIR-V translation and it really happens somewhere near the link phase, but technically it is not linking.
Let's do not confuse anyone and mention something like that: "unsupported types in device code cause errors on SPIR-V translation stage." You can rewrite it in a better way, but please keep it true.

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

erichkeane · 2020-04-07T16:17:46Z

clang/lib/Sema/SemaSYCL.cpp

+  // Not all variable types are supported inside SYCL kernels,
+  // for example the quad type __float128 will cause the resulting
+  // SPIR-V to not link.


Work with @Fznamznon to make sure that this comment means what you want it to.

erichkeane · 2020-04-07T16:20:15Z

clang/lib/Sema/SemaSYCL.cpp

+  if (const auto *CRD = Ty->getAsCXXRecordDecl()) {
+    // If the class is a forward declaration - skip it, because otherwise we
+    // would query property of class with no definition, which results in
+    // clang crash.


Which thing are you querying that would cause this? From the rest of this condition, you simply go through 'fields', which would just be empty if there wasn't a definition, right?

This recursion code, and its comments, was brought over from the recursion in the CheckSYCLType routine that is called by the AST Visitors. I was hoping to leverage the experience there.

But I'll see if I this is strictly necessary here and remove it if not.

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-04-07T16:23:30Z

clang/lib/Sema/SemaSYCL.cpp

+    for (const auto &ParamTy : FPTy->param_types())
+      checkSYCLVarType(S, ParamTy, Loc, Visited);
+    checkSYCLVarType(S, FPTy->getReturnType(), Loc, Visited);
+  } else if (const auto *FTy = dyn_cast<FunctionType>(Ty)) {


Again, curious as to where you found a function type without a prototype in C++ mode! You cannot actually spell one in SYCL/C++. I'm not sure this else-if is worth having.

@erichkeane in retrospect, I'm not sure that any of these additional paths are needed.

If this recursive part was reduced to simply running through the fields of a CXXRecordDecl, and nothing more, would we be overlooking anything? I've been testing function declarations, function pointers, extern "C" structs, etc. and none of them require these paths.

FYI: Just loop through the fields of a RecordDecl (not CXXRecordDecl). Its a little 'shorter' and makes the transition to allowing this code someday in C code lighter (as well as being the 'lowest' type that has the fields you need).

I'm quite surprised the function type doesn't go through there. The CheckSYCLType definitely gets FunctionProtoType going through there(just auto V = &func;), so I'd expect us to have to check that.

FunctionType should NOT get hit, since it isn't possible to create a FunctionType that isn't also a FunctionProtoType[0] (or RecordDecl (that isn't also a CXXRecordDecl) for that matter) in C++.

Basically, Only in C mode do we create RecordDecl (again, that isn't a CXXRecordDecl). Also, it is only possible to spell a Function without a prototype in C. For example:

void foo();

In C, this is a function without a prototype, we don't know the parameter list. In C++ DOES have a prototype, and is equivalent to:

void foo(void);

thanks. that helped, and gave me a new test case as well.
I've tightened up the recursion code considerably, and added a test that verifies function prototypes are getting these type checks applied.

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

erichkeane

1 nit, otherwise I'm happy. @Fznamznon : You happy as well?

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

…duler_docs * origin/sycl: (26 commits) [Driver][SYCL] Move include/sycl header before other system header locations (intel#1492) [BuildBot] Improve usability of buildbot scripts (intel#1472) [NFC] Add GitHub actions badges to README file (intel#1496) [SYCL] Improve error handling for kernel invocation (intel#1209) [SYCL][Driver] Fix SYCL standards' handling for '-fsycl -fsycl-device-only' invocations (intel#1371) [SYCL] Move type checks to later in Semantic Analysis lifecycle (intel#1465) [CI] Download fixed versions of Python tools (intel#1485) [SYCL] Fix sub_group::broadcast (intel#1482) [SYCL][Test] Disable spec_const_redefine.cpp on all devices but HOST (intel#1488) [SYCL] Only export public API (intel#1456) [SYCL][CUDA] Fix selected_binary argument in piextDeviceSelectBinary (intel#1475) [SYCL] Enable LIT testing with CUDA BE (intel#1458) [SYCL] Fix float to half-type conversion (intel#1395) [NFC] Cleanup unneded macro from builtins implementation (intel#1445) Enable cfg-printer LLVM lit tests only if LLVM linked statically (intel#1479) [SYCL][NFC] Reflect the "allowlist" renaming in the code (intel#1480) [SYCL][Doc] Update prerequisites in GetStartedGuide (intel#1466) [SYCL][USM] Remove vestigial dead code (intel#1474) [SYCL-PTX] Fix __spirv_GroupAsyncCopy stride computation (intel#1451) [Driver][SYCL] Emit an error if c compilation is forced (intel#1438) ...

cperkinsintel requested review from bader, Fznamznon and erichkeane April 2, 2020 19:23

erichkeane reviewed Apr 2, 2020

View reviewed changes

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

erichkeane reviewed Apr 2, 2020

View reviewed changes

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

erichkeane reviewed Apr 2, 2020

View reviewed changes

clang/lib/Sema/SemaType.cpp Outdated Show resolved Hide resolved

erichkeane reviewed Apr 2, 2020

View reviewed changes

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

erichkeane reviewed Apr 2, 2020

View reviewed changes

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

erichkeane reviewed Apr 2, 2020

View reviewed changes

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

erichkeane reviewed Apr 2, 2020

View reviewed changes

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

erichkeane reviewed Apr 2, 2020

View reviewed changes

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

erichkeane reviewed Apr 2, 2020

View reviewed changes

cperkinsintel added 2 commits April 2, 2020 14:50

rebased to get past merge conflict

c89f95d

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

moving SYCL type checks out of SemaType so they are called later in t…

961636f

…he Semantic Analysis lifecycle. Signed-off-by: Chris Perkins <chris.perkins@intel.com>

cperkinsintel force-pushed the cperkins-relocate-type-sema branch 2 times, most recently from 2738c55 to cdd61d4 Compare April 2, 2020 23:06

cperkinsintel added 2 commits April 2, 2020 16:13

clang format

4fc329d

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

more clang format

aadb01e

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

cperkinsintel force-pushed the cperkins-relocate-type-sema branch from cdd61d4 to aadb01e Compare April 2, 2020 23:13

cperkinsintel added 3 commits April 2, 2020 16:38

feedback from PR

4aa0cf2

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

testing changes after resolving merge conflict and more PR feedback

c568ed7

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

clang format (no way)

bb43284

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

erichkeane reviewed Apr 3, 2020

View reviewed changes

keryell reviewed Apr 3, 2020

View reviewed changes

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

Fznamznon reviewed Apr 3, 2020

View reviewed changes

incorporate feedback from PR. Added recursive struct member type chec…

eae7e2d

…king, and expanded testing Signed-off-by: Chris Perkins <chris.perkins@intel.com>

cperkinsintel requested review from erichkeane and Fznamznon April 6, 2020 15:34

erichkeane reviewed Apr 6, 2020

View reviewed changes

cperkinsintel added 2 commits April 6, 2020 09:53

incorporating feedback from PR

3b497d3

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

flang-cormat

ff71ec7

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

erichkeane reviewed Apr 6, 2020

View reviewed changes

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

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

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

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

more small changes

9bef479

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

Fznamznon reviewed Apr 7, 2020

View reviewed changes

cperkinsintel requested a review from Fznamznon April 7, 2020 15:36

cperkinsintel added 2 commits April 7, 2020 08:45

merged typecase checsk

0d63fc9

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

braces

a53dcfb

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

cperkinsintel requested a review from erichkeane April 7, 2020 16:09

erichkeane reviewed Apr 7, 2020

View reviewed changes

tighten recursion code, add function proto test, update comment

8e59a2c

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

erichkeane reviewed Apr 7, 2020

View reviewed changes

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

recorDecl instead of recordCXXDecl

2a4ada4

Signed-off-by: Chris Perkins <chris.perkins@intel.com>

erichkeane approved these changes Apr 7, 2020

View reviewed changes

Fznamznon approved these changes Apr 8, 2020

View reviewed changes

bader merged commit 808c5c8 into intel:sycl Apr 8, 2020

cperkinsintel deleted the cperkins-relocate-type-sema branch September 13, 2023 17:08

	void checkSYCLVarDeclIfInKernel(VarDecl *Var);
	void checkSYCLVarDeclInSYCLKernel(VarDecl *Var);

	void checkSYCLVarDeclIfInKernel(VarDecl *Var);
	void diagnoseVarDeclIfSYCLDeviceCode(VarDecl *Var);


		__float128 A; // expected-error {{__float128 is not supported on this target}}
		// ======= Float128 Not Allowed in Kernel ==========

[SYCL] moving type checks to later in Semantic Analysis lifecycle #1465

[SYCL] moving type checks to later in Semantic Analysis lifecycle #1465

Uh oh!

Conversation

cperkinsintel commented Apr 2, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cperkinsintel commented Apr 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fznamznon Apr 7, 2020 •

edited

Loading