[SYCL] Implement SYCL 2020 specialization constants in Clang #3345

elizabethandrews · 2021-03-11T19:23:36Z

This patch implements SYCL 2020 specialization constants in the frontend.

Specialization constants in SYCL 2020 are not captured by lambda and are accessed
through new optional lambda argument kernel_handler. If this argument is present,
a clone of the kernel_handler object should be constructed in openCL kernel.

If the target has no native support for specialization constants, the compiler generates
an openCL kernel argument specialization_constants_buffer of type char*. Generated
kernel_handler clone should then be initialized using the generated kernel argument through __init_specialization_constants_buffer method.

The generated kernel argument specialization_constants_buffer should also have a
corresponding entry in the kernel_signatures structure in the integration header. The param
kind for this argument should be kernel_param_kind_t:specialization_constants_buffer

If the target has native support for specialization constants, the additional argument, and
corresponding handling, need not be generated. In this case, kernel_handler local clone is default
constructed.

Instances of kernel_handler in sycl_kernel is then replaced to use the local clone.

Spec is currently under review here -
#3331

Signed-off-by: Elizabeth Andrews elizabeth.andrews@intel.com

This is a draft patch to get early feedback. Spec is currently under review here - intel#3331 Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

clang/lib/Sema/SemaSYCL.cpp

clang/test/SemaSYCL/kernel-handler.cpp

clang/lib/Sema/SemaSYCL.cpp

erichkeane

Approve of the direction, and don't see anything particularly bad. A couple of comments.

1 thought though.... i wonder if there is value in having multiple kernel_handler parameters (which becomes possible with the loop).

clang/lib/Sema/SemaSYCL.cpp

clang/include/clang/Sema/Sema.h

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2021-03-11T19:34:48Z

clang/lib/Sema/SemaSYCL.cpp

@@ -2871,6 +2970,14 @@ class SyclKernelIntHeaderCreator : public SyclKernelFieldHandler {
    return true;
  }

+  void handleSyclKernelHandlerType(QualType Ty) {
+    // Add corresponding entry in integration header.


Hmm.... I guess the offset is simply nonsensical in this case, right? I wonder if we're better off having some sort of sentinel value to mean that.

I think the runtime team will need to answer that. I do not really know how the integration header is consumed

I know the runtime just loops through these values, but it seems that the offset is simply meaningless/won't be used. From a practicality perspective, pushing 0 here is likely fine. That said (assuming I'm right here), I'd vastly prefer it be an obviously 'meaningless' number. I think that it is unsigned, so -1 doesn't work, so unsigned-max is likely our choice. Please talk with @romanovvlad and confirm I'm correct in its meaninglessness.

I haven't made this change. I thought I would complete a round of testing with 0 and then change it to unsigned-max to see if change causes any break. @romanovvlad mentioned he thinks offset is meaningless based on comment here - https://github.com/intel/llvm/pull/3331/files#r593198110. If that is the case, I think spec should be modified before we change the implementation here

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

…passed Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews · 2021-03-19T19:50:40Z

I will add test for latest commit (parallel_for_work_group) ASAP.

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2021-03-19T13:10:04Z

clang/lib/Sema/SemaSYCL.cpp

+    // TODO: There is no name for the base available, but duplicate names are
+    // seemingly already possible, so we'll give them all the same name for now.
+    // This only happens with the accessor types.
+    StringRef Name = "_arg__base";


For anyone who continues along next time and sees this "TODO" and wants to fix it, the type name of FieldTy might be a good addition here. Duplicates don't cause problems, but perhaps it would be nice from a debug-ability perspective.

erichkeane · 2021-03-19T22:11:15Z

clang/lib/Sema/SemaSYCL.cpp

@@ -777,6 +782,23 @@ constructKernelName(Sema &S, FunctionDecl *KernelCallerFunc,
                                      KernelNameType)};
 }

+static bool hasNativeSycl2020SpecConstantSupport(ASTContext &Context) {


I'd probably prefer something more generic here for the name, like "IsNativeSomethingOrOther".

I do not know what the generic name for targets which satisfy T.isSPIR() && T.getSubArch() == llvm::Triple::NoSubArch is. @bader could you please suggest something

I spoke to @mdtoguchi offline and he suggested isDefaultSYCLArch() or isDefaultSPIRArch(). I plan to go with isDefaultSPIRArch() unless someone here objects :)

What is the meaning of isDefaultSPIRArch() for implementations not using SPIR, like CUDA or others, where they could have an equivalent for specialization constant?

It was my understanding that non-SPIR targets do not have specialization constants support and require special processing. isDefaultSPIRArch() is used to determine when this special processing is required for specialization constants. I do not what the equivalents in CUDA etc are and how/if they are handled in SYCL compiler. @AlexeySachkov @romanovvlad @bader could you please answer?

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

…er change Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

premanandrao

Just a couple minor nits.

clang/lib/Sema/SemaSYCL.cpp

clang/test/CodeGenSYCL/Inputs/sycl.hpp

clang/test/CodeGenSYCL/kernel-handler.cpp

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

clang/test/CodeGenSYCL/kernel-by-reference.cpp

…sitive Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

[WIP][SYCL] Implement SYCL 2020 specialization constants

0db566c

This is a draft patch to get early feedback. Spec is currently under review here - intel#3331 Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews requested a review from erichkeane March 11, 2021 19:24

elizabethandrews commented Mar 11, 2021

View reviewed changes

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

elizabethandrews commented Mar 11, 2021

View reviewed changes

clang/test/SemaSYCL/kernel-handler.cpp Outdated Show resolved Hide resolved

elizabethandrews requested a review from premanandrao March 11, 2021 19:30

elizabethandrews commented Mar 11, 2021

View reviewed changes

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

erichkeane reviewed Mar 11, 2021

View reviewed changes

elizabethandrews mentioned this pull request Mar 12, 2021

[SYCL][Doc] SYCL 2020 specialization constants design #3331

Closed

elizabethandrews added 5 commits March 18, 2021 01:06

Review comments, refactor, corrections, tests

ea11a19

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

Merge remote-tracking branch 'intel_llvm/sycl' into sycl2020specconst

ac62e38

Review comment + fix issues after merge

83c0c65

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

Add kernel_handler type to test headers and correct tests

dbdb906

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

Append _arg__ to kernel argument to be consistent + Minor refactor

7fdf368

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews requested a review from erichkeane March 18, 2021 18:01

erichkeane reviewed Mar 18, 2021

View reviewed changes

elizabethandrews added 2 commits March 18, 2021 13:01

Implement review comments

c1f7cdf

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

IR test

ca9f728

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews changed the title ~~[WIP][SYCL] Implement SYCL 2020 specialization constants~~ [SYCL] Implement SYCL 2020 specialization constants in Clang Mar 18, 2021

elizabethandrews marked this pull request as ready for review March 18, 2021 21:12

elizabethandrews requested review from erichkeane, romanovvlad, kbobrovs, Fznamznon and AlexeySachkov March 18, 2021 21:12

dm-vodopyanov mentioned this pull request Mar 19, 2021

[SYCL] Add DPC++ RT support for SYCL 2020 spec constants (part 1) #3382

Merged

Fix a crash on parallel_for_work_group kernel when kernel_handler is …

d1daf58

…passed Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews changed the title ~~[SYCL] Implement SYCL 2020 specialization constants in Clang~~ [Do not merge][Ready for review][SYCL][Do not merge Implement SYCL 2020 specialization constants in Clang Mar 19, 2021

elizabethandrews changed the title ~~[Do not merge][Ready for review][SYCL][Do not merge Implement SYCL 2020 specialization constants in Clang~~ [Do not merge][SYCL] Implement SYCL 2020 specialization constants in Clang Mar 19, 2021

erichkeane reviewed Mar 19, 2021

View reviewed changes

elizabethandrews added 2 commits March 22, 2021 03:30

Fix crash

4ef981b

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

Implement review comments, add test for PFWG and fix tests after head…

6a43689

…er change Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews changed the title ~~[Do not merge][SYCL] Implement SYCL 2020 specialization constants in Clang~~ [SYCL] Implement SYCL 2020 specialization constants in Clang Mar 23, 2021

erichkeane reviewed Mar 24, 2021

View reviewed changes

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

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

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

Implement review comments

573a98b

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

erichkeane previously approved these changes Mar 24, 2021

View reviewed changes

premanandrao reviewed Mar 24, 2021

View reviewed changes

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

clang/test/CodeGenSYCL/Inputs/sycl.hpp Outdated Show resolved Hide resolved

clang/test/CodeGenSYCL/kernel-handler.cpp Outdated Show resolved Hide resolved

Remove unnecessary define

d1e3854

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews dismissed erichkeane’s stale review via d1e3854 March 24, 2021 21:12

premanandrao previously approved these changes Mar 24, 2021

View reviewed changes

bader reviewed Mar 25, 2021

View reviewed changes

clang/test/CodeGenSYCL/kernel-by-reference.cpp Outdated Show resolved Hide resolved

erichkeane mentioned this pull request Mar 25, 2021

Clang LIT tests using the sycl.hpp header should use "#" syntax instead of @Inputs/sycl.hpp@LINE #3415

Closed

Use markers in header. Update related tests. Make test mangling insen…

55510a9

…sitive Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

elizabethandrews dismissed premanandrao’s stale review via 55510a9 March 26, 2021 06:53

bader merged commit 8e40116 into intel:sycl Mar 26, 2021

elizabethandrews mentioned this pull request Apr 29, 2021

[SYCL] Refactor stream class handing implementation #3646

Merged

[SYCL] Implement SYCL 2020 specialization constants in Clang #3345

[SYCL] Implement SYCL 2020 specialization constants in Clang #3345

Uh oh!

Conversation

elizabethandrews commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elizabethandrews commented Mar 19, 2021

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!

keryell Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elizabethandrews commented Mar 11, 2021 •

edited

Loading

keryell Mar 23, 2021 •

edited

Loading