[SYCL] Wrap pointer fields inside a generated struct #2288

elizabethandrews · 2020-08-10T13:31:55Z

Commit 0b2de9e implemented decomposition of kernel parameters
of struct type i.e. fields of a struct are now individually
passed as separate kernel arguments. This results in an error
for pointer fields of the struct, violating SYCL spec. Copy of structs
with pointers is allowed according to the spec (value of pointer is
undefined). This patch fixes this error by wrapping the pointer
fields in a '__wrapper_class' after the decomposition of struct type.

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

Commit 0b2de9e implemented decomposition of kernel parameters of struct type i.e. fields of a struct are now individually passed as separate kernel arguments. This results in an error for pointer fields, violating SYCL spec. Copy of structs with pointers is allowed according to the spec (value of pointer is undefined). This patch fixes this error by wrapping all pointer fields in a 'wrapper_struct'. Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

jbrodman · 2020-08-10T14:10:45Z

Why are we doing this?

I worry this is going to cause a performance regression.

There only should've been an error decomposing structs if the integration header isn't updated to flag the pointers as kind_pointer instead of kind_std_layout.

Fznamznon · 2020-08-10T13:40:29Z

clang/lib/Sema/SemaSYCL.cpp

+    RecordDecl *WrapperClass =
+        SemaRef.getASTContext().buildImplicitRecord("wrapper_class");


Please add __ prefix to the class name so this class won't conflict with user-defined classes.

Is there value to making it an anonymous struct? Doesn't seem much value to making it named.

I'm not sure. I can take a look.

I didn't change this to use anonymous structs. I don't think it makes a difference in terms of functionality/behavior. Personally I feel naming it makes intention clear. I didn't see a problem with LLVM IR. It handles the names as wrapper_class, wrapper_class.0, etc

clang/lib/Sema/SemaSYCL.cpp

elizabethandrews · 2020-08-10T14:55:19Z

There only should've been an error decomposing structs if the integration header isn't updated to flag the pointers as kind_pointer instead of kind_std_layout.

Can you elaborate on this @jbrodman ?

Why are we doing this?

This change was required because unused pointers in structs were causing an error in the backend when being passed as a direct kernel parameters (after decomposition fields are passed individually. Earlier they were passed inside the struct as a whole). According to the spec, this shouldn't throw an error.

I worry this is going to cause a performance regression

I can look into doing this only for user code where pointers are inside structs, instead of unconditionally doing it for all pointers. That way current USM functionality shouldn't be affected. I think.

erichkeane · 2020-08-10T15:08:08Z

clang/lib/Sema/SemaSYCL.cpp

+    RecordDecl *WrapperClass =
+        SemaRef.getASTContext().buildImplicitRecord("wrapper_class");


Is there value to making it an anonymous struct? Doesn't seem much value to making it named.

clang/lib/Sema/SemaSYCL.cpp

jbrodman · 2020-08-10T15:31:04Z

There only should've been an error decomposing structs if the integration header isn't updated to flag the pointers as kind_pointer instead of kind_std_layout.

Can you elaborate on this @jbrodman ?

Why are we doing this?

This change was required because unused pointers in structs were causing an error in the backend when being passed as a direct kernel parameters (after decomposition fields are passed individually. Earlier they were passed inside the struct as a whole). According to the spec, this shouldn't throw an error.

I worry this is going to cause a performance regression

I can look into doing this only for user code where pointers are inside structs, instead of unconditionally doing it for all pointers. That way current USM functionality shouldn't be affected. I think.

At least in the past, pointers in structs tended to be treated as being generic and not global, and I know this was causing all sorts of problems. If we can make sure all the pointers remain global, even in structs, then that's probably ok?

elizabethandrews · 2020-08-10T15:38:43Z

There only should've been an error decomposing structs if the integration header isn't updated to flag the pointers as kind_pointer instead of kind_std_layout.

Can you elaborate on this @jbrodman ?

Why are we doing this?

This change was required because unused pointers in structs were causing an error in the backend when being passed as a direct kernel parameters (after decomposition fields are passed individually. Earlier they were passed inside the struct as a whole). According to the spec, this shouldn't throw an error.

I worry this is going to cause a performance regression

I can look into doing this only for user code where pointers are inside structs, instead of unconditionally doing it for all pointers. That way current USM functionality shouldn't be affected. I think.

At least in the past, pointers in structs tended to be treated as being generic and not global, and I know this was causing all sorts of problems. If we can make sure all the pointers remain global, even in structs, then that's probably ok?

At the moment, they are yes. Struct decomposition implemented this by 'accident'.

jbrodman · 2020-08-10T15:39:34Z

There only should've been an error decomposing structs if the integration header isn't updated to flag the pointers as kind_pointer instead of kind_std_layout.

Can you elaborate on this @jbrodman ?

Why are we doing this?

This change was required because unused pointers in structs were causing an error in the backend when being passed as a direct kernel parameters (after decomposition fields are passed individually. Earlier they were passed inside the struct as a whole). According to the spec, this shouldn't throw an error.

I worry this is going to cause a performance regression

I can look into doing this only for user code where pointers are inside structs, instead of unconditionally doing it for all pointers. That way current USM functionality shouldn't be affected. I think.

At least in the past, pointers in structs tended to be treated as being generic and not global, and I know this was causing all sorts of problems. If we can make sure all the pointers remain global, even in structs, then that's probably ok?

At the moment, they are yes. Struct decomposition implemented this by 'accident'.

Ok that's probably a net positive then :)

elizabethandrews · 2020-08-10T15:40:06Z

There only should've been an error decomposing structs if the integration header isn't updated to flag the pointers as kind_pointer instead of kind_std_layout.

Can you elaborate on this @jbrodman ?

Why are we doing this?

This change was required because unused pointers in structs were causing an error in the backend when being passed as a direct kernel parameters (after decomposition fields are passed individually. Earlier they were passed inside the struct as a whole). According to the spec, this shouldn't throw an error.

I worry this is going to cause a performance regression

I can look into doing this only for user code where pointers are inside structs, instead of unconditionally doing it for all pointers. That way current USM functionality shouldn't be affected. I think.

At least in the past, pointers in structs tended to be treated as being generic and not global, and I know this was causing all sorts of problems. If we can make sure all the pointers remain global, even in structs, then that's probably ok?

At the moment, they are yes. Struct decomposition implemented this by 'accident'.

There seems to be other issues though. Bunch of USM tests are failing with 'invalid value'. Need to debug this.

bader · 2020-08-10T18:50:13Z

I guess the problem with such pointers and that there are buffers, which runtime can set as kernel arguments, so the solution is to handle them as USM pointers. I expect that runtime just sets host-side pointer value as kernel argument in this case and it should work w/o wrapping the pointer into a struct. Right?

Another option - pointer value can be passed in unsigned integer with size == sizeof (pointer type), but it also prevents compiler memory analysis, so it's better to preserve the type.

elizabethandrews · 2020-08-11T02:18:49Z

I guess the problem with such pointers and that there are buffers, which runtime can set as kernel arguments, so the solution is to handle them as USM pointers. I expect that runtime just sets host-side pointer value as kernel argument in this case and it should work w/o wrapping the pointer into a struct. Right?

Right now those are wrapped in structures as well. From a front-end perspective I don't think we can tell which a USM pointer is vs an invalid pointer, so everything is getting wrapped to avoid the runtime error. I think we can avoid wrapping pointers which are directly captured. That should solve this I think.

bader · 2020-08-11T09:01:16Z

I guess the problem with such pointers and that there are buffers, which runtime can set as kernel arguments, so the solution is to handle them as USM pointers. I expect that runtime just sets host-side pointer value as kernel argument in this case and it should work w/o wrapping the pointer into a struct. Right?

Right now those are wrapped in structures as well. From a front-end perspective I don't think we can tell which a USM pointer is vs an invalid pointer, so everything is getting wrapped to avoid the runtime error. I think we can avoid wrapping pointers which are directly captured. That should solve this I think.

What is "invalid pointer"? I think the compiler assumes that any pointer other than "accessor pointer" is a USM pointer.
If this approach is aligned with what we do USM pointers, I'm okay with it.
But the question is why do we handle USM pointers by wrapping them into a structure? This seems unnecessary. Runtime can pass USM pointer to a kernel directly via clSetKernelArgMemPointerINTEL function.

elizabethandrews · 2020-08-11T14:39:50Z

I guess the problem with such pointers and that there are buffers, which runtime can set as kernel arguments, so the solution is to handle them as USM pointers. I expect that runtime just sets host-side pointer value as kernel argument in this case and it should work w/o wrapping the pointer into a struct. Right?

Right now those are wrapped in structures as well. From a front-end perspective I don't think we can tell which a USM pointer is vs an invalid pointer, so everything is getting wrapped to avoid the runtime error. I think we can avoid wrapping pointers which are directly captured. That should solve this I think.

What is "invalid pointer"? I think the compiler assumes that any pointer other than "accessor pointer" is a USM pointer.
If this approach is aligned with what we do USM pointers, I'm okay with it.
But the question is why do we handle USM pointers by wrapping them into a structure? This seems unnecessary. Runtime can pass USM pointer to a kernel directly via clSetKernelArgMemPointerINTEL function.

Because in the frontend we cannot distinguish between USM and non-USM pointer (invalid pointer). And unused non-USM pointers inside structs which are now being passed directly as kernel argument (after struct decomposition) is causing an error in backend when it should not(Spec allows pointers inside structs).

The proposed solution for this problem was to just wrap all pointers inside a struct and pass them. Upon thinking of this more, I think this is incorrect. We still need to throw an error if an invalid pointer is directly captured. So I'm modifying this patch to not wrap these.

Only pointer fields i.e. fields of struct type kernel arguments are wrapped. Patch also reverts a few test changes I made in previous commit, and adds a test for integration header generation. Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>

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

clang/test/CodeGenSYCL/intel-restrict.cpp

Fznamznon

This one https://github.com/intel/llvm/blob/sycl/clang/test/CodeGenSYCL/pointers-in-structs.cpp will likely fail after your change.

clang/test/CodeGenSYCL/intel-restrict.cpp

clang/test/CodeGenSYCL/pointers.cpp

clang/lib/Sema/SemaSYCL.cpp

elizabethandrews · 2020-08-13T15:33:15Z

This one https://github.com/intel/llvm/blob/sycl/clang/test/CodeGenSYCL/pointers-in-structs.cpp will likely fail after your change.

Yes it did. I modified the test in this commit.

EDIT: 928a7ff

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

1. Correct a comment. 2. Renamed test and wrote description for it. 3. Modified test to use headers and changed RUN line.

Fznamznon

FE changes LGTM, thanks

elizabethandrews · 2020-08-13T19:33:35Z

@intel/llvm-reviewers-runtime This PR has a runtime test. Could you please review?

s-kanaev

The test looks good.

sycl/test/basic_tests/unused_pointer.cpp

…2288) The translator failed assertion with V->user_empty() during regularize function when shl i1 or lshr i1 result is used. E.g. %2 = shl i1 %0 %1 store %2, ptr addrspace(1) @G.1, align 1 Instruction shl i1 is converted to lshr i32 which arithmetic have the same behavior. Original commit: KhronosGroup/SPIRV-LLVM-Translator@239fbd4

[NFC] Update documentation to detail match files

elizabethandrews requested review from Fznamznon, premanandrao and a team as code owners August 10, 2020 13:31

elizabethandrews requested review from rbegam and jbrodman August 10, 2020 13:31

Fznamznon reviewed Aug 10, 2020

View reviewed changes

erichkeane reviewed Aug 10, 2020

View reviewed changes

elizabethandrews marked this pull request as draft August 10, 2020 15:42

elizabethandrews changed the title ~~[SYCL] Wrap all pointers inside a generated struct~~ [WIP][SYCL] Wrap all pointers inside a generated struct Aug 10, 2020

elizabethandrews added 3 commits August 12, 2020 12:37

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

926e0a1

Fix failing test after merge

928a7ff

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

elizabethandrews commented Aug 13, 2020

View reviewed changes

clang/test/CodeGenSYCL/intel-restrict.cpp Outdated Show resolved Hide resolved

elizabethandrews marked this pull request as ready for review August 13, 2020 13:33

elizabethandrews changed the title ~~[WIP][SYCL] Wrap all pointers inside a generated struct~~ [SYCL] Wrap all pointers inside a generated struct Aug 13, 2020

elizabethandrews requested a review from Fznamznon August 13, 2020 14:26

Fznamznon reviewed Aug 13, 2020

View reviewed changes

Revert unrelated ClangFormat changes.

61feacc

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

Applied review comments:

fe6181a

1. Correct a comment. 2. Renamed test and wrote description for it. 3. Modified test to use headers and changed RUN line.

elizabethandrews mentioned this pull request Aug 13, 2020

[SYCL][NFC] Apply clang format to a test #2314

Merged

Fznamznon approved these changes Aug 13, 2020

View reviewed changes

elizabethandrews changed the title ~~[SYCL] Wrap all pointers inside a generated struct~~ [SYCL] Wrap pointer fields inside a generated struct Aug 13, 2020

s-kanaev approved these changes Aug 14, 2020

View reviewed changes

sycl/test/basic_tests/unused_pointer.cpp Show resolved Hide resolved

bader merged commit 926eb32 into intel:sycl Aug 14, 2020

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025

Merge pull request #2288 from RossBrunton/ross/condocs

31cb7d9

[NFC] Update documentation to detail match files

		RecordDecl *WrapperClass =
		SemaRef.getASTContext().buildImplicitRecord("wrapper_class");

[SYCL] Wrap pointer fields inside a generated struct #2288

[SYCL] Wrap pointer fields inside a generated struct #2288

Uh oh!

Conversation

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

Uh oh!

Uh oh!

jbrodman commented Aug 10, 2020

Uh oh!

Fznamznon Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

erichkeane Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elizabethandrews commented Aug 10, 2020

Uh oh!

erichkeane Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbrodman commented Aug 10, 2020

Uh oh!

elizabethandrews commented Aug 10, 2020

Uh oh!

jbrodman commented Aug 10, 2020

Uh oh!

elizabethandrews commented Aug 10, 2020

Uh oh!

bader commented Aug 10, 2020

Uh oh!

elizabethandrews commented Aug 11, 2020

Uh oh!

bader commented Aug 11, 2020

Uh oh!

elizabethandrews commented Aug 11, 2020

Uh oh!

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elizabethandrews commented Aug 13, 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!

elizabethandrews commented Aug 13, 2020

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elizabethandrews commented Aug 10, 2020 •

edited

Loading

elizabethandrews commented Aug 13, 2020 •

edited

Loading