Core: Make PropagateNaN min/max agree on signed-zero ties

Summary

pmin/pmax<PropagateNaN> did not select the same operand on a signed-zero tie as plain pmin/pmax of the same type: the SSE/AVX/AVX512 helper passed op its operands reversed, while PropagateFast and PropagateNumbers did not. The two rules meet inside every reduction, so the sign of a zero result depended on where the vectorized body stopped and the scalar tail began. Sign of v.maxCoeff<PropagateNaN>() for a vector of +0.0 with a single -0.0 last:

n=1 2 3 4 5 6 7 8 9
SSE4.2, float − + + − + + + − +
SSE4.2, double − − + − + − + − +
AVX2, float − + + + + + + − +
AVX2, double − + + − + + + − +

-0.0 appears exactly where n is a multiple of the packet size — the cases with no scalar tail, where the packet rule wins outright. Identical under GCC 13 and Clang 18, so the divergence is in Eigen rather than a compiler artifact.

The contract

pmin/pmax<PropagateNaN> may differ from plain pmin/pmax only where a NaN is involved. Given two ordered operands both must select the same one, down to the sign of a zero result. Signed zeros are the only operands that compare equal while differing in their bits, so that is what this pins down.

Which operand a tie selects stays deliberately unspecified. It varies between backends, and between the scalar and packet forms of one backend, wherever the hardware min/max resolves a tie by sign rather than by position — see Follow-up.

The fix

The generic path passes op its operands in the caller's order, and SSE's pminmax_propagate_nan tests b rather than a. op mirrors std::min/std::max and already returns a when either operand is NaN, so testing b is exactly what supplies the missing case, and it leaves op's operand order — hence its tie — intact.

NEON, RVV10, LSX and the generic clang backend already route PropagateNaN through their plain op and are unchanged; the clang backend's pmin_nan_op is already written this way.

Every row of the table above becomes − + + + + + + + +, identically for both types under both ISAs, and PropagateFast and PropagateNumbers now agree with it: maxCoeff() returns -0.0 only when the -0.0 is the first element.

Cost

One instruction per packet in a vectorized PropagateNaN reduction — testing the freshly loaded operand stops it folding into vmaxpd's memory operand. Callgrind instruction counts, 200 reductions of 4096 doubles, -O3 -mavx2 -mfma:

path gcc 13 clang 18
v.maxCoeff<PropagateNaN>() +4.63% +4.67%
v.cwiseAbs().maxCoeff<PropagateNaN>() 0.00% 0.00%
c.real().cwiseAbs().maxCoeff<PropagateNaN>() 0.00% (byte-identical) 0.00%
c.realView().cwiseAbs().maxCoeff<PropagateNaN>() 0.00% +7.17%

Those percentages are relative to that harness's total. Re-measured since from a standalone probe, the absolute figure is the one that travels: maxCoeff<PropagateNaN>() over 4096 doubles costs +204,400 (gcc 13) and +204,600 (clang 18) instruction references across 204,800 packets — 0.998 and 0.999 instructions per packet — and exactly 0 once cwiseAbs() puts the operand in a register.

bench_reductions registers these paths as of da2a3768, so the cost and any later recovery are measurable rather than resting on the Callgrind recipe above: PropagateNaN and PropagateNumbers rows for vector minCoeff/maxCoeff, and cwiseAbs(), real().cwiseAbs() and realView().cwiseAbs() rows with and without PropagateNaN, over float, double, complex<float> and complex<double>. PropagateNumbers is the control: it goes through the sibling wrapper, which this change does not touch. The PropagateFast registrations and their names are unchanged, since PropagateFast is the default template argument of minCoeff()/maxCoeff().

Recovering it would mean swapping operand order in the shared Redux.h loops, for the scalar and packet paths alike, which also moves every reduction's tie from the earliest to the latest index and changes which NaN payload sum/prod propagate. That belongs in its own change.

Test

packetmath gained packetmath_minmax_propagation_test, which checks the invariant within each type — the scalar form and every packet form — over signed zeros, signed infinities and NaN. Zero results are compared bit-exactly, because isApprox cannot distinguish +0.0 from -0.0; NaN payloads are not pinned across backends, so a NaN result only has to stay a NaN.

The check fails with either half of the fix reverted, verified independently for both.

It deliberately does not compare the scalar form against the packet form. That is what could not hold on NEON, and it is not what this change establishes.

PropagateNumbers is checked for its NaN semantics only: under EIGEN_VECTORIZE_GENERIC it is reached through __builtin_elementwise_minnum, whose zero-sign tie is specified independently of the plain compare-select form, so pinning it to plain pmin would assert something Eigen does not claim.

The existing coverage could not catch any of this: the PropagateNaN coefficient-wise check fed only {NaN, +0.0} — never -0.0 — and compared through isApprox.

Second commit: document realView()

realView() carried no documentation at all — DenseBase declared it without a Doxygen block and no page mentioned it — so the one component view that can be linear and vectorizable was undiscoverable from the strided views it can replace. It is now documented and added to the \sa lists of real() and imag().

The claims are scoped to a complex *this, since for a real scalar both real() and realView() return the source expression itself. For a complex *this, real() and imag() build a CwiseUnaryView, which drops PacketAccessBit, so a reduction over either runs coefficient by coefficient; RealView::Flags intersects the source flags, so linear access, packet access and writability survive only where the source expression supplies them, and writing additionally requires std::complex storage.

Validation

packetmath (15 splits) green in four configurations: GCC 13 at -msse4.2 and at -mavx2 -mfma, Clang 18 at -mavx2 -mfma, and Clang 18 at -mavx2 -mfma -DEIGEN_VECTORIZE_GENERIC=1. array_cwise (41), redux (13), stable_norm (13) and vectorization_logic (2) green in the first three — 207 further runs. reuse lint and codespell clean, clang-format-17 applied, Doxygen reports nothing on the changed block.

The new bench_reductions registrations are verified to reach the changed helper by codegen, not by timing: under -O3 -mavx2 -mfma every PropagateFast instantiation emits plain vminpd/vmaxpd with no compare or blend, while every PropagateNaN and PropagateNumbers instantiation emits vcmppd plus vblendvpd. They compile under GCC 13 at -msse4.2, -mavx2 -mfma and -mavx512f -mavx512dq -mavx512vl -mfma, and under Clang 18 at -mavx2 -mfma with and without EIGEN_VECTORIZE_GENERIC=1. They have not been timed: this host is under unrelated load, and the Callgrind figures above remain the reproducible record until it is quiet.

Not run locally: aarch64 and riscv64, for want of a cross toolchain on this host. The argument that they go green is by construction — their PropagateNaN specializations forward to their plain op, so the invariant holds there without a code change — rather than by execution.

Follow-up

#3116 tracks the backends where the packet path resolves a tie by sign while the scalar path resolves it by position, so a reduction's zero sign still depends on the packet width there: NEON and RVV10, which reach pmin/pmax through vminq/vfmin, and GPU builds, where numext::mini/maxi are specialized to number-preferring fmin/fmax. Recovering the instruction above by reordering the min/max redux accumulation is a second follow-up.

Found while re-measuring #3097 (closed).

Edited by Rasmus Munk Larsen

Merge request reports

Loading
Loading