search

stfc / PSycloneBench

Various benchmarks used to inform PSyclone optimisations
BSD 3-Clause "New" or "Revised" License
6 stars 5 forks source link

Update NemoLite2d manual C OpenCL to use the C PSy layer #45

Closed sergisiso closed 4 years ago

sergisiso commented 4 years ago

Issue #33

Update the C OpenCL implementations in single_file_c_opencl to use the current c_family kernels and the Fortran nemolite2d algorithm and infrastructure, while keeping the invoke in C through the Fortran->C interface. The updated implementations are located in psykal_c_opencl.

This will allow to keep them up to date more easily (I could remove a bunch of headers and c files that duplicated infrastructure code) and compare the correctness and performance with the other implementations as they now reuse the common build system. Additionally we could implement a OpenCL Tasks by using the time_step.c as a base.

Also, the new version works with arbitrary problem sizes and OpenCL local_sizes and OpenCL does not crash with bigger problem sizes.

There were a few OpenCL C implementations:

I started with just the all kernels implementation, now in time_step.c, the other c files are placed in the old_versions folder, they don't work with current infrastructure/kernels but I think they are worth keeping in master because the channels are not implemented anywhere else.

sergisiso commented 4 years ago

@arporter This is ready for review, it's the C OpenCL version I was talking about. Essentially I took a big part of what was already in single_file_c_opencl and brought it up to date with a few modifications.

sergisiso commented 4 years ago

@arporter I addressed your comments, this is ready for next review.

arporter commented 4 years ago

Almost there now. Please could you just add a comment to each of the kernel files explaining that they are in-lined into xxx and that file then has the necessary OpenCL includes. Once that's done I shall merge.

sergisiso commented 4 years ago

@arporter I decided to revert this particular change as I realized the kernel shouldn't necessarily know about this details of how it is imported. I hope this is a good solution. I marked this PR ready for review again.

arporter commented 4 years ago

I'm happy now - I think putting back the includes was the right thing to do.