Implement parallelization for hydrogen bond list setup in gfnff_hbset and gfnff_hbset0

[foxtran]

Now, subroutines gfnff_hbset and gfnff_hbset0 now are parallelized using OpenMP. The idea is to collect information about connections in OpenMP-thread-specific arrays and then dump it to global arrays. I did not test it a lot, but tests now 1.5 s faster on my laptop for 4 threads, mainly from xtb/gfnff test.

I've also reformatted source code of these routines.

Alternative approach is #1240.

Closes #1239
Closes #1262

[Copilot]

Pull request overview

This PR implements OpenMP parallelization for the hydrogen bond list setup routines gfnff_hbset and gfnff_hbset0 in the GFN-FF force field implementation. The approach uses thread-local buffers to collect connection information independently in each thread, then merges the results into global arrays using critical sections.

Key changes:

• File extension changed from .f90 to .F90 to enable preprocessor directives for conditional OpenMP compilation
• gfnff_hbset parallelized with thread-local buffers and dynamic merging
• gfnff_hbset0 parallelized using OpenMP reduction for simple counting

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/gfnff/meson.build	Updated build file to reference renamed .F90 file for preprocessing support
src/gfnff/CMakeLists.txt	Updated CMake build file to reference renamed .F90 file for preprocessing support
src/gfnff/gfnff_ini2.F90	Implemented OpenMP parallelization with thread-local buffers, critical sections for merging, and code reformatting

Comments suppressed due to low confidence (17)

src/gfnff/gfnff_ini2.F90:1348

• The nlist parameter is declared as intent(in) but is not used in the subroutine body. This appears to be a leftover from refactoring. Consider removing this unused parameter or documenting why it's needed for the API.
  src/gfnff/gfnff_ini2.F90:784
• Typo in comment: 'occured' should be 'occurred' (double 'r').
  src/gfnff/gfnff_ini2.F90:927
• The array shape declaration hblist(5, nhb) is incorrect. The nhb parameter is modified during execution (reset to 0 when OpenMP is active), but array dimensions in Fortran must be fixed at the time of the subroutine call. When nhb is used as a dimension, it creates an assumed-shape array with the size based on the value at call time. However, the intent is to use nhb as a counter. The declaration should be hblist(:,:) (assumed-shape) or hblist(5, *) (assumed-size) to properly handle the variable-length data.
  src/gfnff/gfnff_ini2.F90:939
• The array shape declaration hblist(5, nxb) is incorrect. The nxb parameter is modified during execution (reset to 0 when OpenMP is active), but array dimensions in Fortran must be fixed at the time of the subroutine call. When nxb is used as a dimension, it creates an assumed-shape array with the size based on the value at call time. However, the intent is to use nxb as a counter. The declaration should be hblist(:,:) (assumed-shape) or hblist(5, *) (assumed-size) to properly handle the variable-length data.
  src/gfnff/gfnff_ini2.F90:781
• The N_MAX_LIST constant (800 elements = ~32KB) deserves better documentation. The comment says "keep approx. 32 kb of integer(int64)" but the code uses default integers which are typically 32-bit (4 bytes), not 64-bit (8 bytes). For 800 elements × 5 values × 4 bytes = 16KB per thread per list (not 32KB). If memory usage is a concern, this discrepancy should be clarified. Also, document the rationale for choosing 800 as the buffer size.
  src/gfnff/gfnff_ini2.F90:786
• The early return condition logic is confusing. The condition .not.(rmsd < 1.d-6 .or. rmsd > 0.3d0) is equivalent to rmsd >= 1.d-6 .and. rmsd <= 0.3d0, meaning it returns when RMSD is in the normal range [1e-6, 0.3] and continues when RMSD is either very small (< 1e-6, first call) or very large (> 0.3, substantial move). Consider rewriting this for clarity, e.g., if (rmsd >= 1.d-6 .and. rmsd <= 0.3d0) return with a comment explaining the logic.
  src/gfnff/gfnff_ini2.F90:941
• The array assignment statement is commented out with !$, but it's essential for copying thread-local data to the global list. Without this line, only the counter is updated while the actual data is never transferred from the thread-local hblist to neigh_list%hblist3. This will result in incorrect/uninitialized data in the halogen bond list.

The line should be active when OpenMP is enabled. Change !$ to !$omp or remove the !$ prefix entirely to make it execute in the critical section.
src/gfnff/gfnff_ini2.F90:845

• Buffer overflow logic is incorrect. The sequence is: (1) increment nhb2, (2) write to hblist2(*, nhb2), (3) check if nhb2 == N_MAX_LIST and flush. This means when nhb2 reaches N_MAX_LIST, the element at index N_MAX_LIST has already been written. The check should happen BEFORE incrementing and writing, or the buffer size should be N_MAX_LIST + 1. Otherwise, this will write beyond the allocated array bounds when nhb2 reaches 800.
  src/gfnff/gfnff_ini2.F90:915
• The array shape declaration hblist(5, nhb) is incorrect. The nhb parameter is modified during execution (reset to 0 when OpenMP is active), but array dimensions in Fortran must be fixed at the time of the subroutine call. When nhb is used as a dimension, it creates an assumed-shape array with the size based on the value at call time. However, the intent is to use nhb as a counter. The declaration should be hblist(:,:) (assumed-shape) or hblist(5, *) (assumed-size) to properly handle the variable-length data.
  src/gfnff/gfnff_ini2.F90:817
• Typo in comment: 'adjustet' should be 'adjusted'.
  src/gfnff/gfnff_ini2.F90:1372
• Typo in comment: 'adjustet' should be 'adjusted'.
  src/gfnff/gfnff_ini2.F90:917
• The array assignment statement is commented out with !$, but it's essential for copying thread-local data to the global list. Without this line, only the counter is updated while the actual data is never transferred from the thread-local hblist to neigh_list%hblist1. This will result in incorrect/uninitialized data in the hydrogen bond list.

The line should be active when OpenMP is enabled. Change !$ to !$omp or remove the !$ prefix entirely to make it execute in the critical section.
src/gfnff/gfnff_ini2.F90:929

• The array assignment statement is commented out with !$, but it's essential for copying thread-local data to the global list. Without this line, only the counter is updated while the actual data is never transferred from the thread-local hblist to neigh_list%hblist2. This will result in incorrect/uninitialized data in the hydrogen bond list.

The line should be active when OpenMP is enabled. Change !$ to !$omp or remove the !$ prefix entirely to make it execute in the critical section.
src/gfnff/gfnff_ini2.F90:858

• Buffer overflow logic is incorrect. The sequence is: (1) increment nhb2, (2) write to hblist2(*, nhb2), (3) check if nhb2 == N_MAX_LIST and flush. This means when nhb2 reaches N_MAX_LIST, the element at index N_MAX_LIST has already been written. The check should happen BEFORE incrementing and writing, or the buffer size should be N_MAX_LIST + 1. Otherwise, this will write beyond the allocated array bounds when nhb2 reaches 800.
  src/gfnff/gfnff_ini2.F90:870
• Buffer overflow logic is incorrect. The sequence is: (1) increment nhb1, (2) write to hblist1(*, nhb1), (3) check if nhb1 == N_MAX_LIST and flush. This means when nhb1 reaches N_MAX_LIST, the element at index N_MAX_LIST has already been written. The check should happen BEFORE incrementing and writing, or the buffer size should be N_MAX_LIST + 1. Otherwise, this will write beyond the allocated array bounds when nhb1 reaches 800.
  src/gfnff/gfnff_ini2.F90:893
• Buffer overflow logic is incorrect. The sequence is: (1) increment nxb, (2) write to hblist3(*, nxb), (3) check if nxb == N_MAX_LIST and flush. This means when nxb reaches N_MAX_LIST, the element at index N_MAX_LIST has already been written. The check should happen BEFORE incrementing and writing, or the buffer size should be N_MAX_LIST + 1. Otherwise, this will write beyond the allocated array bounds when nxb reaches 800.
  src/gfnff/gfnff_ini2.F90:772
• The neigh parameter is declared as intent(inout) but it's only read in this subroutine (used to access neigh%transVec, neigh%bpair, neigh%numctr, neigh%nTrans, etc.). It should be declared as intent(in) for clarity and to prevent accidental modifications. This also affects the OpenMP shared clause correctness.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

[foxtran]

Well... I do not see any other data races in modified fragment of xtb. Since the different number of iteration happens even in main branch, I do not think that I want to debug them in this PR.

[thfroitzheim]

Let's open an issue for the different number of iterations in the geometry optimization.