PathMSDBase: drop four omp simd pragmas that cannot vectorize - #1437
Merged
carlocamilloni merged 1 commit intoJul 30, 2026
Merged
Conversation
Building with clang and OpenMP emits, four times:
PathMSDBase.cpp:160:19: warning: loop not vectorized: the optimizer was
unable to perform the requested transformation ... [-Wpass-failed=transform-warning]
All four remarks are attributed to calculate()'s opening line, which hides which
loops are at fault. Isolating them by keeping one pragma at a time shows exactly
four contributors, and two pragmas that are fine:
line 176 imgVec[i].property = ... ; imgVec[i].index = i; 1 warning
line 234 tmp_derivs2[i*nat+j] = tmp_derivs[j]; 1 warning
line 244 (the same loop again) 1 warning
line 266 (the same loop again) 1 warning
line 335 derivs_s[i] += tmp*it.distder[i]; 0 - vectorizes
line 340 derivs_z[i] += ...; 0 - vectorizes
Neither failing case can be vectorized, and asking for it is misleading:
- Line 176 is not merely hard to vectorize, it is meaningless. ImagePath::property
is a std::vector<double>, so `imgVec[i].property = indexvec[i]` is a vector
copy-assignment that ALLOCATES. There is no SIMD work in that loop.
- The other three are the same contiguous copy of Vector elements written out by
hand. std::copy_n says what is meant, and lets the compiler emit a memmove
instead of an element loop the vectorizer then declines to transform.
Fixing the source rather than silencing the diagnostic means the warning is gone
for every compiler, and a build using -Werror no longer fails here. Behaviour is
unchanged: std::copy_n over [0,nat) is exactly the loop it replaces, and the
remaining two pragmas, which do vectorize, are untouched.
Verified with clang 20.1.1:
clang++ -std=c++17 -O3 -fopenmp -march=skylake-avx512 -Wall \
-c colvar/PathMSDBase.cpp -I.
4 warnings before, 0 after. GCC is unaffected either way (its 17 warnings here
are pre-existing -Wunknown-pragmas from `#pragma acc` in headers, which PLUMED's
own build silences).
7 tasks
carlocamilloni
pushed a commit
that referenced
this pull request
Jul 31, 2026
…#1438) * RMSD: reformulate two loops so their omp simd pragmas can be honoured A plain `./configure && make` fails on any machine whose default C++ compiler is clang: configure enables OpenMP automatically, and src/tools/Makefile appends -Werror, so the vectorizer's refusal is fatal. RMSD.cpp:1443:35: error: loop not vectorized: the optimizer was unable to perform the requested transformation; the transformation might be disabled or specified as part of an unsupported transformation ordering [-Werror,-Wpass-failed=transform-warning] RMSD.cpp:1487:35: error: (the same, in getDDistanceDReference) Both are attributed to the opening line of the enclosing function, which hides the loop at fault. Each of the two functions contains exactly one `#pragma omp simd`, so the attribution is unambiguous: the loops at lines 1479 and 1530. The pragma is kept. The loop body is rewritten so the compiler can act on it: the loop-invariant `(ddist_dc*-csum)` is hoisted into a named Vector, and the Vector expression is expanded into its three components. Both loops now vectorize at width 4, confirmed with -Rpass=loop-vectorize, instead of being refused. Alternatives measured with clang 20.1.1, all on the same tree: braces around the if body still fails hoisting the invariant only still fails binding `Vector& dv=derivatives[iat]` still fails flatten to `double*` + manual 3-way unroll vectorizes, but the pointer is UB when n==0 per-component `derivatives[iat][k]` vectorizes <- chosen The Vector arithmetic creates temporaries the vectorizer cannot see through, and naming the element through a reference reintroduces them; only the direct per-component form works. No other pragma in the file is touched. The other eight active ones already vectorize. Two more, on identically shaped std::vector<Vector> loops at lines 1339 and 1426, were commented out long ago because they gave wrong results with the Intel compiler; these two had the same shape. Behaviour is unchanged, and the 17 regtests that exercise these paths pass, including rt-rmsd-displace, which is the align != displace case that actually enters the modified branch. This is the same class of problem as #1437. It escapes CI because no job builds clang with OpenMP enabled: the Linux workflow uses gcc and icpx, and macsimple uses AppleClang without libomp, where `#pragma omp simd` is inert. * RMSD: also unswitch getDistance's reduction loop The first commit fixed the two loops that fail at -O3. Building PLUMED with mpicxx at -O2 shows a third one, which -O3 happens to vectorize: RMSD.cpp:1361:22: error: loop not vectorized: the optimizer was unable to perform the requested transformation [-Werror,-Wpass-failed=transform-warning] The remark is attributed to getDistance()'s opening line; the pragma is the `omp simd reduction(+:localDist)` at line 1374. Its body branches on alEqDis and safe, both of which are loop-invariant members, so the loop carries a per-iteration test the vectorizer will not always unswitch by itself. Hoisting the two branches out of the loop makes it vectorize at -O2 as well. This is a pure unswitching: both conditions are invariant, the summation order is unchanged, and the arithmetic is untouched, so results are bit-identical. Measured on this file with clang 20.1.1, counting -Wpass-failed diagnostics (-march makes no difference at any level): -O level before after -O0 0 0 -O1 5 4 -Os 8 7 -O2 3 0 -O3 2 0 -O1 and -Os remain non-clean, but not for a reason any source change can fix: clang's loop vectorizer is effectively disabled there, so every `omp simd` in the file is refused, including the ones that are perfectly good at -O2. Making PLUMED buildable at those levels would mean not combining -Werror with -Wpass-failed in the module makefiles; that is a build-system decision and is deliberately left out of this PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Building PLUMED with clang and OpenMP emits this four times:
All four are attributed to
calculate()'s opening line, which hides which loops are responsible.Keeping one
#pragma omp simdat a time and recompiling isolates them exactly:imgVec[i].property = indexvec[i]; imgVec[i].index = i;tmp_derivs2[i*nat+j] = tmp_derivs[j];derivs_s[i] += tmp*it.distder[i];derivs_z[i] += ...;Neither failing case can be vectorized, and requesting it is misleading:
ImagePath::propertyis astd::vector<double>, soimgVec[i].property = indexvec[i]is a vector copy-assignment thatallocates. There is no SIMD work in that loop.
Vectorelements, written out by hand.std::copy_nstates the intent and lets the compiler emit amemmoverather than an element loopthe vectorizer then declines to transform.
Fixing the source rather than silencing the diagnostic means the warning disappears for every
compiler, and a build that adds
-Werrorno longer fails here. Behaviour is unchanged:std::copy_nover
[0, nat)is exactly the loop it replaces, and the two pragmas that do vectorize are untouched.Net change: 7 insertions, 13 deletions in one file.
Target release
I would like my code to appear in release 2.11 (master).
Type of contribution
Copyright
author of the code I am modifying.
Tests
No new regtest is added: this changes neither behaviour nor interface, so there is nothing new to
assert. The existing PathMSD regtests are the check, and they already encode the expected colvar and
derivative values. Locally, with the change applied, all seven that this build can run pass:
Verified with clang 20.1.1:
4 warnings before, 0 after. GCC is unaffected either way.