OpenCL Kernel Generation in SemaSYCL.cpp needs significant refactoring-

[erichkeane]

The code to construct the OpenCL kernel (Basically everything from ConstructOpenCLKernel down) as well as integration headers is quite a mess. We need to refactor/split a number tasks into their own functions, and simplify the implementation significantly.

CreateOpenCLKernelBody and buildArgTys are particularly bad.

[erichkeane]

BTW: I'm using this bug to keep track of our efforts here, so a bit of my comments here will be 'stream of conciousness' as I go along.

First, we use large lambdas stuffed into std::functions WAY too often.

Second, we seem to loop around the fields of the lambda about 3-4 times on the way through this, with horrifyingly similar (albeit different) implementations. It seems to me that we need to loop these fields once, and do what needs to be done there.

Third, we have a surprising amount of 'special casing' all over the place that is concerning. I can't help but pray that these slightly different implementations haven't diverged too much.

Finally, our comments seem to be out of sync in quite a few places, and not particularly descriptive when they exist.

I'm considering a couple of mechanisms here, we might wish to create a visitor type that each 'visit' implements based on the fields to deal with the customization. it would be really nice to only loop 1x across all of these. Assembling an object as we go would be a good start. I'll think more overnight and might have a chance to take a look tomorrow.

[erichkeane]

Hmm... BuildArgTys seems to have quite a few responsibilities that I'm not sure are all in its socpe. it seems to be doing type checking as well for the parameters list.

Also, this concept of a "special SYCL Obj" is odd, I'll ahve to figure out more of what that means beyond the accessor (and possibly samplers, though there is a TODO to consider that).

It would be really nice if we could create a type to hold all of the fields and their general dispositions instead of running through a few times. Its a touch of a shame all the work that CreateOpenCLKernel has to do that seems to be a repeat of the previous times through. It is also a shame it isn't made to operate without the OpenCL Kernel functiondecl already being created.

I don't quite understand the Accessor type, so I'll have to figure tha tout too.

[erichkeane]

A little more thinking... ConstructOpenCLKernelBody ONLY needs the declaration context of the function as far as I can see? We can change the type of the functoindecl after it is constructed (or as it is constructed), which might be worth trying.

[kbobrovs]

• @Fznamznon, the author of the original ConstructOpenCLKernelBody (AFAIK)

[kbobrovs]

CreateOpenCLKernelBody and buildArgTys are particularly bad.

seems like their evolution went the wrong way...

[erichkeane]

CreateOpenCLKernelBody and buildArgTys are particularly bad.

seems like their evolution went the wrong way...

Absolutely... The whole set of functions seems to have diverged and evolved in the opposite way the original author thought they would. As I get time the next few days, I'll start a high-level refactor with limited functionality that I may ask you (@kbobrovs) and @Fznamznon to help out with. I noticed even the unqiue-stable-name implementation ended up in what I would consider the 'wrong' place, so hopefully we can wrangle this back.

[elizabethandrews]

Second, we seem to loop around the fields of the lambda about 3-4 times on the way through this, with horrifyingly similar (albeit different) implementations. It seems to me that we need to loop these fields once, and do what needs to be done there.

Very basic algorithm -

1. Loop over fields to do required processing (based on type) and create OpenCLKernel Arguments. (buildArgTys)
2. Loop over fields to generate local clone and assign it from 'processed' arguments created in 1. (CreateOpenCLKernelBody)

As far as I understand, the 'processing' in step 1. above is all for various OpenCL limitations.

@Fznamznon had suggested doing away with the local clone if we're passing the whole object as a parameter, when discussing refactor with me a couple of weeks backs. I'm not sure how to handle the special fields and pointers if we do so however.

[Fznamznon]

@Fznamznon had suggested doing away with the local clone if we're passing the whole object as a parameter, when discussing refactor with me a couple of weeks backs. I'm not sure how to handle the special fields and pointers if we do so however.

I think we will still have to do some special handling for special fields as we do now, but number of required kernel parameters will be reduced.

[Fznamznon]

BTW, I heard that OpenMP and maybe CUDA has some additional code generation like we have for generation of OpenCL kernel in SemaSYCL. But they do somewhere inside the CodeGen instead of Sema. We can try to investigate what is happening there, maybe we have to reconsider our approach to OpenCL kernel generation at all.

[elizabethandrews]

As far as I understand, the 'processing' in step 1. above is all for various OpenCL limitations.

https://github.com/intel/llvm/blob/sycl/sycl/doc/CompilerAndRuntimeDesign.md#lowering-of-lambda-function-objects-and-named-function-objects
"To facilitate the mapping of SYCL kernel data members to OpenCL kernel arguments and overcome OpenCL limitations we added the generation of an OpenCL kernel function inside the compiler. An OpenCL kernel function contains the body of the SYCL kernel function, receives OpenCL-like parameters and additionally does some manipulation to initialize SYCL kernel data members with these parameters."

[erichkeane]

See #1495 for my first run at this.