Can't it be replaced with closure_nodes.size()?

Not sure why it's needed?

Doesn't this belong to kernel_types.h?

It's also a bit dangerous because of possible padding added to ShaderData. Need some clearer way of calculating the size, but needs some deeper thoughts.

It should at least be an enum, not a string.

Also, why exactly it is so specific? Can't cache just cache whatever program is requested?

Same as above.

Not needed.

Prefer to have output parameters at the end of the list.

Guess it is something to be fixed in the compiler as well. It's also important ot mention what exact version/vendor of compiler is broken.

Also, comments is a full sentence, starting with capital and having fullstop at the end.

We really need to simplify this with some macro, like:

#define KERNEL_APPEND_ARG(kernel, arg, narg) \
  opencl_assert(clSetKernelArg(kernel, narg++, sizeof(arg), (void*)&arg));

KERNEL_APPEND_ARG(ckFilmConvertKernel, d_data, narg);
KERNEL_APPEND_ARG(ckFilmConvertKernel, d_rgba, narg);
/* ...and so on... */

Could also be wrapped into some smarter macro which will do CUDA-like kernel launching. Or just an inlined c++ function with some template magic:

template <typename T1, typename T2, typename T3, typename T4 ...>
inline kernel_launch(kernel, T1 a, T2 b ...) {
  int narg = 0;
  KERNEL_APPEND_ARG(kernel, a, narg);
 /* ... */
}

/* Launch film kernel. */
kernel_launch(ckFilmConvertKernel, d_data, d_rgba, d_buffer);

KERNEL_TEX is to be undefined to this point.

Undefine KERNEL_TEX.

If kernel itself needs to know if it's split or not, then better to negotiate this flag and call it SPLIT_KERNEL. This way no changes will be needed to keep CPU/CUDA working.

Undefine KERNEL_TEX.

Remove _SPLIT_KERNEL suffix.

Undefine KERNEL_TEX. And actually do it in all similar cases.

break is meaningless in this case, by removing this redundant statements you'll clean up this function quite a lot.

Also return could be at the same line, making the function even smaller.

Would be nice to avoid this all together, but currently i can't think of more clear way. Finding better way of dealing with this could be mentioned in the TODO tho.

assert(!"Uknown node type passed to FOO")

If it can't be 0 assert for that and don't check of it being zero.

Same as above.

Not really sure, will it survive if one closure is replaced with another one in shader tree, keeping overall number of closures the same?

Would prefer if all defines passed via cflags were wrapped with underscores.

Seems __SPLIT_KERNEL, max closure options and work stealing seem could be deduplicated in this function.

yeah . it definitely makes sense to put it in DeviceInfo. I will make the change

closure_nodes lists all the svm nodes that will ever be traversed in "svm_eval_nodes()". clos_max denotes how big the "MAX_CLOSURE" macro should be, for the current scene. In megakernel if "MULTI_CLOSURE" is enabled, the "MAX_CLOSURE" value is default to 64. Not all of the 64 locations in ShaderData's "closure" array is necessary to render a scene.

use_split_kernel flag is mainly used in load_kernels logic. If the device is going to use split kernel, then we delay load_kernels() execution to after "device_update()". (Note : load_kernels() depends on data from device_update() to realize "selective svm node compilation" optimization)

It was present in kernel_types.h. But I moved it to device_opencl.cpp because, this macro is also used in device_opencl.cpp.
My rationale is that, one would expect cycles to work if he/she disabled/enabled (that is, after a build) a feature-macro in kernel_types.h (build/bin/Release/2.74/scripts/addons/cycles/kernel/kernel_types.h). is it not so ? work_stealing could be thought of as an optimization-feature. However, if one disabled work_stealing in kernel_types.h (in the bin folder), then cycles would crash since "WORK_STEALING" related code will still be enabled in the binary executable.

@Martijn Berger (juicyfruit) : But this is the best-fitting place i could find for these declarations :( any ideas ?
@Sergey Sharybin (sergey) : Yeah, It could be made better. Needs some thought.

I will make the change into an enum.

We have the string to denote which program it should return. Previously i.e. in blender-master branch, we had only one program and get_program, set_program will implicitly refer to that program only. Since we have 2 programs here, we need to explicitly tell get_program and set_program about what program we are refering to.

noted. will remove it.

yeah. will make necessary changes.

This comment is not introduced in cycles_kernel_split commits.
Will make note of the comment format and make changes accordingly.
Question on comment format : If the sentence is too big, it could be broken into 2 or more lines, right ?

I think the first approach, set of macros to set kernel arguments followed by a enqueue_kernel, is clean, clear and straight-forward.
However, its your call. do you want it done using templates ?

noted. will make necessary changes.

noted. will make necessary changes.

The flow now is that, the load_kernels of OpenCLDeviceBase will compile filmconvert, shader and bake kernels.
If the device is to work on megakernel, then the load_kernels of OpenCLDeviceMegakernel will compile only kernel_ocl_path_trace kernel.
If the device is to work on splitkerenl, then the load_kernels of OpenCLDeviceSplitkernel will compile all the split kernels of path trace.

The flag "COMPILE_ONLY_MEGAKERNEL" is used in OpenCLDeviceMegakernel to prevent recompilation of other kernels. This flag also prevents un-necessary compilation of megakernel in OpenCLDeviceBase's load_kernels().

Also this flag is only available in kernel.cl and would not harm CPU/CUDA's working.

noted. will make necessary changes.

noted.will make necessary changes.

noted. will make necessary changes.

yeah ok. i will clean it up and write a TODO.

noted. will make necessary changes.

My mistake, It should have been "shouldn't" instead of "can't" in both cases. I will correct the comments.

Let me consult with another developer and get back to you on this.

noted. will make necessary changes.

yeah. will de-duplicate them.

noted. will make necessary changes.

ok.

noted. will use macros for kernel enqueue.

@Sergey Sharybin (sergey), @Martijn Berger (juicyfruit) : will change it to ccl_addr_space.

Not sure how this "XXX" got introduced. I will remove it.

I believe its possible to remove duplication. Let me check it out further and get back to you.

the split kernel version has one extra argument. let me check out the de-duplication feasibility and get back to you on this .

noted. will make necessary changes.

I searched for a ccl_local definition and could't find one. I will add a definition and replace all "__local" with "ccl_local".

Let me check it out in detail and get back to you.

The split kernel version has extra arguments (sd and isect_global). This function can be de-duplicated by adding the two extra arguments to the original version and subsequently changing all the function calls.

ok. will make necessary changes.

I will check it out and get back to you.

yes.

yeah ok. will make necessary changes.

ok . will make necessary changes.

will make necessary changes.

yeah . got it :)

This is the tile size set by the user in the gui. I was not aware that it could be calculated already. I will check it out.

This value is calculated in thread_run and used inside path_trace. Thinking about it, I believe,This probably belongs somewhere in device_opencl.cpp. will check it out.

sorry, I dont understand. Where is the variable "closure" that you have mentioned ? This value is used in the calculation of clos_max in Device class.

Will need a little help here. We will need a better understanding of svm_nodes compilation to do it ourselves without relying on device_update().
The idea is that we should find all the svm_nodes that would be traversed in "svm_eval_nodes" function here and enable appropriate macros.
Is there some doc that you could share regarding that ?

you mean, a shader node could be compiled differently for each render of a same scene ?

Not needed anymore.

TODO: We can split kernel.cl into kernel_path_trace.cl and kernel_common.cl and then we can get rid of this enum. But could happen after the merge.

Should be if(

This could also be gone after the kerbel.cl split.

This could be replaced with inlined sizeof(), same as it is done for stuff like ShaderData.