RMSD: reformulate two loops so their omp simd pragmas can be honoured - #1438
Conversation
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 plumed#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.
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.
|
Pushed a second commit: a third loop in this file needed the same treatment, and I had missed it because the failure set depends on the optimization level. Building PLUMED with Counting
So the PR now makes the file clean at both optimization levels where the vectorizer actually runs.
Re-verified after the second commit: full regtest suite, 824 tests, same 20 pre-existing failures as master with |
|
@carlocamilloni Should this PR go to master as #1437 did or better go to |
2 participants
Description
./configure && make, with no arguments at all, fails on any machine whose default C++ compiler isclang.
configureauto-detects OpenMP, andsrc/tools/Makefileappends-Werror, so thevectorizer's refusal to honour a
#pragma omp simdis fatal: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 isunambiguous: the loops at lines 1479 and 1530, which are the same loop written twice.
The pragma is kept. Rather than deleting it, the loop body is rewritten so the compiler can act
on it: the loop-invariant
(ddist_dc*-csum)is hoisted into a namedVector, and theVectorexpression is expanded into its three components. Both loops now vectorize at width 4 — confirmed
with
-Rpass=loop-vectorize— instead of being refused.I measured the alternatives rather than guessing, all with clang 20.1.1 on the same tree:
ifbodyVector& dv=derivatives[iat]in the bodydouble*+ manual 3-way unrolln==0derivatives[iat][k]The
Vectorarithmetic creates temporaries the vectorizer cannot see through, and naming theelement through a reference reintroduces them; only the direct per-component form works. Hoisting
alone is not sufficient, but it is necessary, so both changes are needed together.
No other pragma in the file is touched. The other eight active ones already vectorize — their
enclosing functions produce no remark. Two more, on identically shaped
std::vector<Vector>loopsat lines 1339 and 1426, were commented out long ago with the note that they "lead to incorrect
results with INTEL compiler"; the two fixed here had exactly that shape, so this also removes a
latent instance of that hazard.
Why CI did not catch it
No job builds clang with OpenMP enabled.
linuxWF.ymluses gcc andicpx;macWF.yml'smacsimplejob uses AppleClang withoutlibomp, so#pragma omp simdis inert there and-Wno-unknown-pragmashides the rest. gcc has no-Wpass-faileddiagnostic at all. The defaultbuild on a clang host is the configuration nobody exercises.
This is the same class of problem as #1437, found by scanning the whole tree for it afterwards.
omp simdoccurs in only two files in the entire repository —src/colvar/PathMSDBase.cpp, fixedby #1437, and
src/tools/RMSD.cpp— and there are no#pragma unroll,#pragma clang loop,ivdeporvectordirectives anywhere. With this change, an--enable-modules=allbuild is freeof
-Wpass-failedentirely.Target release
I would like my code to appear in release 2.11 (master).
v2.10carries the same two pragmas but not the-Werrorinsrc/tools/Makefile, which arrivedon master only. There it is a warning rather than a build failure, so this is not a forward-merge
candidate unless you would like the warning silenced there too.
Type of contribution
Copyright
the 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 tests are the check, and they already encode the expected colvars and
derivatives.
Locally I ran the full suite twice against the same tree, once with the change and once with
src/tools/RMSD.cppreverted to master, so the comparison isolates this file:The failure lists are byte-identical, so this change introduces none and hides none. All 20 are
pre-existing and unrelated: 4 are
basic/rt-make-*, which compile against an installed plumed(this tree was never
make installed), and 16 areves/rt-bf-*basis-function tests. The runenables every module (
--enable-modules=all) with--disable-mpi, which is why the count isnon-zero at all.
The 17 regtests that exercise these code paths all pass, including
rt-rmsd-displace— thealign != displacecase, which is the branch the modified loops actually sit in — andrt65-rmsd2,rt-close-structureandrt64-pca, the three named in the Intel comment above.Builds verified:
./configure && make(clang auto-detected, OpenMP auto-enabled, default modules) — failsbefore this change, clean after: 380 files, 0 errors.
--enable-modules=allwith clang + OpenMP +-O3 -march=skylake-avx512— clean, 0-Wpass-failedacross all 56 modules.
make -C astyleleaves the file unchanged.