Improve openMP variables and macros to avoid unnecessary warnings #6230
Conversation
THe gmt_M_grd_loop and similar macros result in tons of warnings due to the problem with Windows not supporting OpenMP > 2.0 and forces us to have signed loop variables. This PR addresses this by introducing a typedef openmp_int that is set accordingly, and we use casts to keep things quiet regardless of what openmp_int is.
|
Do you want to run the full tests on this PR before merging or just go based off the few simple tests run on all PRs? Running the full tests can be done by uncommenting L7 in the tests workflow (https://github.com/GenericMappingTools/gmt/blob/no-warnings/.github/workflows/tests.yml). We would need to recomment that line before merging. The CI tests take a few hours to run. |
|
The tests workflow had two new failures on linux and windows, though I was not able to reproduce these failures locally on linux: The full test results are at https://github.com/GenericMappingTools/gmt/actions/runs/1720120943. I'll revert the tests workflow. |
Co-authored-by: Meghan Jones <[email protected]>
Co-authored-by: Meghan Jones <[email protected]>
|
OK, I will work some more on grdlandmask then. |
Sorry, I wasn't clear. I do not think you need to do more work on grdlandmask. There are still failures on windows, but they are identical on master and this branch so they are most likely unrelated to the openMP implementation. The grdlandmask tests pass. I'm checking linux now and then will approve. |
|
OK, I misunderstood - too much multitasking and zooms at the same time. |
|
I ran into issues with the tests hanging - I think on firmoviscous.sh so I am holding off on approving until I look into it more. |
|
I fixed a few things in grdmask related to OpenMP. Now, when I compare the test results from the MP build with the non-OpenMP build I get the same 12 failures in master (related to lines). So I think nothing is broken in the PR. However, I learned that the grdmask with OpenMP looping over the few nodes inside a small search radius is much slower than non-OpenMP due to the overhead of setting up the extra threads just to compute a few points. So either we need to decide during run time that the size of the radius is large enough to warrant doing it or we skip the openMP part. That would mean an if-test and near-duplicate coding somewhere. Given the speed up due to #6253 I think we should just yank the OpenMP part from the -S section. |
Sounds good. Probably the easiest way to resolve the merge conflicts too. |
|
Let me know if this latest version is acceptable - no warnings on my side and I removed one OpenMP loop from grdmask (kept the one involving polygons). |
The gmt_M_grd_loop and similar macros result in tons of warnings due to the problem with Windows not supporting OpenMP > 2.0 and therefore forces us to have signed loop variables. Because GMT internals, including published API structures, uses unsigned variables for quantities that cannot be negative, this means we either have to add many casts or risk not noticing legitimate warnings in a sea of false positives.
This PR addresses this quandary by introducing a typedef openmp_int that is set to
intunder Windows andunsigned intelsewhere, and we use casts to keep things quiet regardless of what openmp_int is. I use openmp_int wherever those macro-loops are used, wherever we use OpenMP, and in places were we may want to add OpenMP in the future.I have tested this branch for both signed and unsigned typedef and all warnings have gone away. All tests that pass in master still pass in both compilations. Before approving and merging we should see if anything funny is discovered by @joa-quim and/or the CI tests outside macOS.