search

stfc / PSycloneBench

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

Use grid%subdomain methods to get the NemoLite2D Fortran loop boundaries #37

Closed sergisiso closed 3 years ago

sergisiso commented 4 years ago

Manual Fortran implementations were doing an extra row and column of unnecessary work in some kernels. This PR updates the loop boundaries to use the current 'dl_esm_inf' infrastructure to get the sizes.

Also I took the psykal_serial as a use case to extend the Makefile generation of result tables for cases that there is more than one implementation inside a single folder.

sergisiso commented 4 years ago

@arporter This is not ready yet (the loop boundaries modification should be done in multiple places) but could you have a look how I implemented it here and let me know what you think before I start replicating this in various places.

What I did is re-use (and also simplify) some variables that were commented out a the top of the manual PSy-layer to fix the different boundary values, and then use this variables in the loop constructs. This is slightly different that C++/Kokkos were I put the boundary expression in-place in the loop construct. Which approach do you think is better/more readable?

(I noticed the u/v_momentum still does an extra row in C++ I will fix that too 😅 )

arporter commented 4 years ago

I like what you've done and I think having the variable names makes it more readable (as opposed to adjusting the loop limits in-place). In theory (and hopefully in practice) we shouldn't need to use NBOUNDARY anymore - there should be enough information in the grid object?

sergisiso commented 4 years ago

In theory (and hopefully in practice) we shouldn't need to use NBOUNDARY anymore - there should be enough information in the grid object?

Just to be sure, you mean change the whole_xstart = xstart - NBOUNDARY expressions for whole_xstart = ssha%grid%subdomain%whole%xstart ?

sergisiso commented 4 years ago

@arporter Ups ssha%grid%subdomain%whole does not exist, 'whole' just exist for fields. Looking at the psykal_dm version it uses ssha%internal%xstart and ssha%whole%xstart, should I use these ones instead of going trough the grid structure?

arporter commented 4 years ago

@sergisiso, yes, I expect that the psykal_dm version is correct - it at least sounds right that we use 'whole' rather than 'internal'! TBH, I can't remember why it is only fields that have this and not the grid but that's not something to worry about now. It might be worth looking in the code to check precisely what NBOUNDARY means (I can't remember :-( ) to check that using field%whole%xstart is consistent.