xcompact3d / x3d2

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

Remove unused variables from OMP build #48

Closed pbartholomew08 closed 6 months ago

pbartholomew08 commented 6 months ago

There are unused dummy arguments, but they may be required by generic interface, or are not yet implemented.

These variables were found by setting CMAKE_Compiler_FLAGS to

-g -std=f2008 -Wall -Wpedantic -Werror -Wimplicit-interface -Wimplicit-procedure -Wno-unused-dummy-argument

in the CMake configuration.

We should aim to do development with (similar) flags to ensure code quality.

pbartholomew08 commented 6 months ago

I realise I've probably broken the "open an issue" workflow, but wanted to show why we should consider enabling these flags. If you think it's worth doing/discussing there then I'll close and do things properly.

Nanoseb commented 6 months ago

Yes, happy with that. We should consider making these flags the default at least in debug mode with GNU.

slaizet commented 6 months ago

agreed!

pbartholomew08 commented 6 months ago

I'll add these default options and see if we have equivalents for Nvidia in Xcompact3d.

Nanoseb commented 6 months ago

Sorry one last thing. It seems the github actions aren't testing the debug build (not sure which one is the default). So we might want to add -DCMAKE_BUILD_TYPE=Debug in the config line there.

semi-h commented 6 months ago

Can we still use below to activate debug mode? https://github.com/xcompact3d/x3d2/blob/c6cc95b1b637cdea1990894371d15eff0ee4c0eb/docs/source/developer/build.rst?plain=1#L16

pbartholomew08 commented 6 months ago

Can we still use below to activate debug mode?

https://github.com/xcompact3d/x3d2/blob/c6cc95b1b637cdea1990894371d15eff0ee4c0eb/docs/source/developer/build.rst?plain=1#L16

Yes, the only change is that we are adding some flags over -g

pbartholomew08 commented 6 months ago

Note I had to change how some coefficients were stored in the OMP tridiagonal solver. I think this is because they were being set as a local variable. This would work within a PARALLEL PRIVATE(du_s, du_e) region, however with multiple SIMD regions I'm not sure how the local storage works. I've gone with replacing this with explicit storage of size SZ (the vector length?) which should cost no more than the assumed vector-private storage I think.

semi-h commented 6 months ago

Note I had to change how some coefficients were stored in the OMP tridiagonal solver. I think this is because they were being set as a local variable. This would work within a PARALLEL PRIVATE(du_s, du_e) region, however with multiple SIMD regions I'm not sure how the local storage works. I've gone with replacing this with explicit storage of size SZ (the vector length?) which should cost no more than the assumed vector-private storage I think.

Well spotted! I copied this from the CUDA version, added simd loops and obviously forgot to fix this bit. I don't think simd region can itself extend the single variable on the left to a vector sized one and store it that way, the way it was coded is just wrong. Maybe this was even preventing vectorisation, haven't really checked.

pbartholomew08 commented 6 months ago

Spotted by the compiler - not me!

pbartholomew08 commented 6 months ago

I believe this is good to go. I've added additional debug flags to the NVHPC build based on 2decomp. For either backend we can select between these by setting CMAKE_BUILD_TYPE either at initial configuration or using ccmake