WIP Replace local clone with SYCL kernel object #1568

elizabethandrews · 2020-04-22T04:17:32Z

Added Kernel Object to KernelDecl and Integration header.

erichkeane

@Fznamznon I very much want your input here.

Also, this needs tests!

I see that we're adding this to the header, but not the body of the kernel. Is that intentional?

erichkeane · 2020-04-22T13:12:52Z

clang/lib/Sema/SemaSYCL.cpp

                              Handlers &... handlers) {
+
+  QualType KernelType = QualType(KernelObject->getTypeForDecl(), 0);


Are these only allowed to be top-level things? Can you clarify how this feature is supposed to work? I don't get it yet.

We're adding the kernel object as a parameter of the kernel function (just like we did with the fields). Currently in the kernel body, a kernel object clone is generated and initialized using the fields of kernel function. The clone is then subsequently used in kernel body code. We're now exploring the possibility of getting rid of the clone entirely and using this kernel object

clang/lib/Sema/SemaSYCL.cpp

elizabethandrews · 2020-04-22T14:53:53Z

@Fznamznon I very much want your input here.

Also, this needs tests!

Yes. I'll be fixing lit tests and adding any required tests in a followup patch. After that I'll work on removing the clone and replacing it's usage in kernel body with this. I just pushed this patch to start getting feedback on approach :)

I see that we're adding this to the header, but not the body of the kernel. Is that intentional?

We'll be using this in the body instead of the local clone. I didn't tackle that in this patch.

Fznamznon

I don't have objections yet.

Fznamznon · 2020-04-22T14:46:32Z

clang/lib/Sema/SemaSYCL.cpp

@@ -830,6 +845,10 @@ class SyclKernelFieldChecker
            << 1 << FieldTy;
    }
  }
+  void handleKernelObject(CXXRecordDecl *KernelObject,
+                          QualType KernelType) final {
+    // Do we need any diagnostics for Kernel Object?


Since kernel object can be defined not only as lambda but as a callable user's type, we could validate that the kernel object is trivially copy constructible (or satisfies standard layout requirement). It is required by the spec (section 4.8.9.1)

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews · 2020-04-27T04:12:21Z

Sorry for the delay. I was having A LOT of trouble running the tests locally. Finally got the environment issues fixed!

This patch just fixes current lit fails. Actual body changes have not been covered in this patch. I pushed the patch to check if passing the kernel object as a top level parameter would break any test/cause regressions. I worked briefly on the body changes on Friday. I will pick it up again tomorrow. Hopefully will push another patch by EOD or Tuesday.

bader · 2020-04-27T07:42:17Z

@elizabethandrews, please, apply clang-format to your patch and remove WIP from the PR title when the patch is ready for review.

This patch removes the local clone generated in kernel. In the kernel body, it replaces any usage of the clone with that of the kernel object passed as a parameter to the kernel. Since we are now directly using the kernel object, scalar arguments (except pointers) and struct type arguments (except those containing accessor fields) need not be generated and passed to kernel. This code has been removed. Pointers require additional 'handling' for USM. For these, arguments are still generated and passed to kernel. Initialization of clone pointer fields have been replaced by assignment of kernel object pointer fields from these parameters. Please note accessors and streams have not been handled yet. So related tests will fail as required initializations are now missing. Signed-off-by: Elizabeth Andrews <[email protected]>

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews · 2020-04-29T05:07:36Z

clang/test/CodeGenSYCL/spir-calling-conv.cpp

@@ -9,7 +9,9 @@ int main() {



The IR for this code has simplified from -

; Function Attrs: norecurse nounwind define spir_kernel void @_ZTSZ4mainE15kernel_function() #0 !kernel_arg_addr_space !4 !kernel_arg_access_qual !4 !kernel_arg_type !4 !kernel_arg_base_type !4 !kernel_arg_type_qual !4 { entry: %0 = alloca %"class._ZTSZ4mainE3$_0.anon", align 1 %1 = bitcast %"class._ZTSZ4mainE3$_0.anon"* %0 to i8* call void @llvm.lifetime.start.p0i8(i64 1, i8* %1) #3 %2 = addrspacecast %"class._ZTSZ4mainE3$_0.anon"* %0 to %"class._ZTSZ4mainE3$_0.anon" addrspace(4)* call spir_func void @"_ZZ4mainENK3$_0clEv"(%"class._ZTSZ4mainE3$_0.anon" addrspace(4)* %2) %3 = bitcast %"class._ZTSZ4mainE3$_0.anon"* %0 to i8* call void @llvm.lifetime.end.p0i8(i64 1, i8* %3) #3 ret void } to ; Function Attrs: norecurse nounwind define spir_kernel void @_ZTSZ4mainE15kernel_function(%"class._ZTSZ4mainE3$_0.anon"* byval(%"class._ZTSZ4mainE3$_0.anon") align 1 %_arg_kernelObject) #0 !kernel_arg_addr_space !4 !kernel_arg_access_qual !5 !kernel_arg_type !6 !kernel_arg_base_type !6 !kernel_arg_type_qual !7 { entry: %0 = addrspacecast %"class._ZTSZ4mainE3$_0.anon"* %_arg_kernelObject to %"class._ZTSZ4mainE3$_0.anon" addrspace(4)* call spir_func void @"_ZZ4mainENK3$_0clEv"(%"class._ZTSZ4mainE3$_0.anon" addrspace(4)* %0) ret void }

@Fznamznon Are those lifetime markers needed? I'm not sure what the intrinsic really does and if it's required.

Lifetime markers are needed to specify start/stop of memory objects life time (oops I'm a captain obvious here). Lifetime markers define kinda frame outside of which the memory object is dead and has undefined value (see https://llvm.org/docs/LangRef.html#id2407 , it can explain better).
In the first LLVM IR snippet I see lifetime markers only for local kernel object clone. If we are going to remove local kernel object clone, then no need to keep lifetime markers for it :)
I expect exactly the same simplification in resulting LLVM IR which I see in your second snippet, so it is a nice progress.

bader · 2020-05-17T09:36:57Z

@elizabethandrews, are you still working on this patch?

elizabethandrews · 2020-05-17T14:56:17Z

No. We've abandoned this effort since whole object cannot be passed without breaking openCL spec

…#1568) Original commit: KhronosGroup/SPIRV-LLVM-Translator@42cf770

elizabethandrews requested review from erichkeane and Fznamznon as code owners April 22, 2020 04:17

elizabethandrews mentioned this pull request Apr 22, 2020

Pass whole SYCL kernel object as OpenCL kernel argument #1544

Closed

erichkeane reviewed Apr 22, 2020

View reviewed changes

Fznamznon reviewed Apr 22, 2020

View reviewed changes

elizabethandrews added 2 commits April 22, 2020 08:44

[SYCL] Add kernel object to Kernel function and integration header

b2f3950

Signed-off-by: Elizabeth Andrews <[email protected]>

Fix lit failures

3eae199

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews force-pushed the replace_clone branch from 557ea88 to 3eae199 Compare April 27, 2020 04:05

elizabethandrews added 3 commits April 28, 2020 21:34

Clang format changes

cce4455

Signed-off-by: Elizabeth Andrews <[email protected]>

Resolved merge conflicts

11aa9a2

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews commented Apr 29, 2020

View reviewed changes

Fix build error

4b8a799

elizabethandrews closed this May 17, 2020

bb-sycl pushed a commit that referenced this pull request Sep 15, 2022

Use TypedPointerType in lieu of PointerIndirectPair in mangleBuiltin. (…

9d06835

…#1568) Original commit: KhronosGroup/SPIRV-LLVM-Translator@42cf770

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

WIP Replace local clone with SYCL kernel object #1568

WIP Replace local clone with SYCL kernel object #1568

Uh oh!

elizabethandrews commented Apr 22, 2020 •

edited

Loading

Uh oh!

erichkeane left a comment

Uh oh!

erichkeane Apr 22, 2020

Uh oh!

elizabethandrews Apr 22, 2020

Uh oh!

Uh oh!

elizabethandrews commented Apr 22, 2020 •

edited

Loading

Uh oh!

Fznamznon left a comment

Uh oh!

Fznamznon Apr 22, 2020

Uh oh!

elizabethandrews commented Apr 27, 2020

Uh oh!

bader commented Apr 27, 2020

Uh oh!

elizabethandrews Apr 29, 2020 •

edited

Loading

Uh oh!

Fznamznon May 6, 2020 •

edited

Loading

Uh oh!

bader commented May 17, 2020

Uh oh!

elizabethandrews commented May 17, 2020

Uh oh!

Uh oh!

		Handlers &... handlers) {

		QualType KernelType = QualType(KernelObject->getTypeForDecl(), 0);

WIP Replace local clone with SYCL kernel object #1568

WIP Replace local clone with SYCL kernel object #1568

Uh oh!

Conversation

elizabethandrews commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elizabethandrews commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Fznamznon Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

elizabethandrews commented Apr 27, 2020

Uh oh!

bader commented Apr 27, 2020

Uh oh!

elizabethandrews Apr 29, 2020 • 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!

Fznamznon May 6, 2020 • 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!

bader commented May 17, 2020

Uh oh!

elizabethandrews commented May 17, 2020

Uh oh!

Uh oh!

elizabethandrews commented Apr 22, 2020 •

edited

Loading

elizabethandrews commented Apr 22, 2020 •

edited

Loading

elizabethandrews Apr 29, 2020 •

edited

Loading

Fznamznon May 6, 2020 •

edited

Loading