Skip to content

Commit 675f720

Browse files
authored
RMSD: reformulate two loops so their omp simd pragmas can be honoured (plumed#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 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. * 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.
1 parent e44cef6 commit 675f720

1 file changed

Lines changed: 22 additions & 9 deletions

File tree

src/tools/RMSD.cpp

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1371,13 +1371,16 @@ double RMSDCoreData::getDistance( const bool squared) {
13711371
} else {
13721372
localDist=eigenvals[0]+rr00+rr11;
13731373
}
1374-
#pragma omp simd reduction(+:localDist)
1375-
for(unsigned iat=0; iat<n; iat++) {
1376-
if(alEqDis) {
1377-
if(safe) {
1374+
if(alEqDis) {
1375+
if(safe) {
1376+
#pragma omp simd reduction(+:localDist)
1377+
for(unsigned iat=0; iat<n; iat++) {
13781378
localDist+=align[iat]*modulo2(d[iat]);
13791379
}
1380-
} else {
1380+
}
1381+
} else {
1382+
#pragma omp simd reduction(+:localDist)
1383+
for(unsigned iat=0; iat<n; iat++) {
13811384
localDist+=displace[iat]*modulo2(d[iat]);
13821385
}
13831386
}
@@ -1475,11 +1478,16 @@ std::vector<Vector> RMSDCoreData::getDDistanceDPositions() {
14751478
}
14761479
}
14771480

1478-
if(!alEqDis)
1481+
if(!alEqDis) {
1482+
const Vector shift=ddist_dcpositions-csum;
14791483
#pragma omp simd
14801484
for(unsigned iat=0; iat<n; iat++) {
1481-
derivatives[iat]= prefactor*(derivatives[iat]+(ddist_dcpositions-csum)*align[iat]);
1485+
const double a=align[iat];
1486+
derivatives[iat][0]=prefactor*(derivatives[iat][0]+shift[0]*a);
1487+
derivatives[iat][1]=prefactor*(derivatives[iat][1]+shift[1]*a);
1488+
derivatives[iat][2]=prefactor*(derivatives[iat][2]+shift[2]*a);
14821489
}
1490+
}
14831491

14841492
return derivatives;
14851493
}
@@ -1526,11 +1534,16 @@ std::vector<Vector> RMSDCoreData::getDDistanceDReference() {
15261534
}
15271535
}
15281536

1529-
if(!alEqDis)
1537+
if(!alEqDis) {
1538+
const Vector shift=ddist_dcreference-csum;
15301539
#pragma omp simd
15311540
for(unsigned iat=0; iat<n; iat++) {
1532-
derivatives[iat]= prefactor*(derivatives[iat]+(ddist_dcreference-csum)*align[iat]);
1541+
const double a=align[iat];
1542+
derivatives[iat][0]=prefactor*(derivatives[iat][0]+shift[0]*a);
1543+
derivatives[iat][1]=prefactor*(derivatives[iat][1]+shift[1]*a);
1544+
derivatives[iat][2]=prefactor*(derivatives[iat][2]+shift[2]*a);
15331545
}
1546+
}
15341547

15351548
return derivatives;
15361549
}

0 commit comments

Comments
 (0)