search

xcompact3d / x3d2

https://xcompact3d.github.io/x3d2
BSD 3-Clause "New" or "Revised" License
3 stars 4 forks source link

Reorder for OMP backend #68

Closed Nanoseb closed 5 months ago

Nanoseb commented 6 months ago

closes #42 closes #64

Nanoseb commented 5 months ago

This is ready for review. I am not 100% happy with having to send the block sizes around like that, but I have opened a separate issue to deal with it #77. Also I am not 100% sure about having this select case inside tight loops. I would expect compilers to notice the check is always constant and not bother checking it. But if it becomes a problem I will have to write separate functions for each direction (which I tried to avoid here).

Nanoseb commented 5 months ago

Also note that the tests written here are meant to also run on the cuda backend but I haven't tried them because I don't have the hardware at hand.

Nanoseb commented 5 months ago

All looks good. As @semi-h has mentioned extending the tests to cover CUDA I will leave approving for now.

The tests I think are sensible in that they check the reordering is reversible. However to ensure that they are indeed working, I would suggest

1) reorder a field from Cartesian into Directional/vectorised layout
2) use the Fortran intrinsic to reshape this into a 1D layout
3) check that neighbouring values in the Cartesian field are separated by SZ steps in the vectorised layout

Actually, thinking about it, it will work for the particular data layout we are using now, but won't when we switch to not having SZ cells on a single line (which we are planning to do to reduce padding). So not sure it is worth it to create a test for it now.

Reversibility is the easiest to tests these things, but indeed it is flawed if you make the same mistake twice. Something I could do is have more convoluted ways to go back to the original field, that way it is less prone to errors cancelling each other out. I also can test if all the indices are 'touched' by the mapping once and only once by each function. I believe all of this should cover most edge cases that could arise.

pbartholomew08 commented 5 months ago

All looks good. As @semi-h has mentioned extending the tests to cover CUDA I will leave approving for now. The tests I think are sensible in that they check the reordering is reversible. However to ensure that they are indeed working, I would suggest

1) reorder a field from Cartesian into Directional/vectorised layout
2) use the Fortran intrinsic to reshape this into a 1D layout
3) check that neighbouring values in the Cartesian field are separated by SZ steps in the vectorised layout

Actually, thinking about it, it will work for the particular data layout we are using now, but won't when we switch to not having SZ cells on a single line (which we are planning to do to reduce padding). So not sure it is worth it to create a test for it now.

Reversibility is the easiest to tests these things, but indeed it is flawed if you make the same mistake twice. Something I could do is have more convoluted ways to go back to the original field, that way it is less prone to errors cancelling each other out. I also can test if all the indices are 'touched' by the mapping once and only once by each function. I believe all of this should cover most edge cases that could arise.

OK, I thought the data structure was quite frozen, maybe we should stick with the reordering as is for now - it is a good test as you say. Future commits can always extend the tests. Ideally the test we want shouldn't rely directly on the data structure but this is easier said than done