Add toDouble for the FastInt family - #6594
Conversation
The pre-filters that reject a case in double before evaluating it exactly need a bignum-to-double conversion, which the family lacked: MSVC's std::_Signed128 has no such operator at all. It is correctly rounded, and deliberately platform-independent rather than deferring to the built-in __int128_t conversion, since the stated error bound is what makes those rejections safe.
| const FastUInt128 u( v ); | ||
| // deliberately the same code as for FastInt below, and not the built-in conversion of | ||
| // __int128_t, which MSVC's std::_Signed128 lacks: the value must not depend on the platform | ||
| return detail::doubleFromWords( std::array{ std::uint64_t( u ), std::uint64_t( u >> 64 ) } ); |
There was a problem hiding this comment.
why not built-in conversion of __int128_t?
- is not it faster?
- do we expect different conversion results for some int128 values?
mrbind emitted MR_toDouble(__int128 v), which no build accepts: gcc rejects __int128 in a header compiled with -pedantic-errors, and MSVC has no such keyword at all. FastInt128 has no C spelling, and the rest of the family is already ignored for the same reason.
|
"why not built-in conversion of Measured both, 1) Yes, the built-in is faster — 8.0 vs 13.3 ns/conversion (1.66×). Neither is inline: the built-in calls libgcc 2) No divergence found. 6.56M values compared bit-for-bit against the built-in: hand-picked edges (0, ±1, 2^53, 2^53+1, 2^64+2^11, INT128_MIN/MAX), 20k random values at each of the 127 widths, and the cases that would actually expose a double rounding — exact ties and tie±1 at all 74 widths above 2^53, both round-to-even directions. 0 mismatches, so libgcc's So the case for our own code is only the platforms I can't measure on this box: MSVC's Your call, and it decides what I do next:
Separately — this PR broke CI on every platform, now fixed in 63b0541. mrbind generated No UI surface, nothing to drive in the viewer. The header compiles clean with g++-12 in the ubuntu22 image, and the two measurement programs above run against it. |
|
keep our conversion for MSVC only, but remove |
Per review: only MSVC lacks a conversion in double, so only it needs our own code. There the libm call was about a third of the cost of the conversion, and a bit-pattern power of two replaces it with the same bits. A new assertion in FastIntToDouble128 pins the two implementations to each other on every platform. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both done in 1575757.
One correction to the timings I gave above. Measured again over 2M values, built-in vs ours reverses: built-in 12.1 ns, ours 7.0 ns. The built-in is an out-of-line Verification
|
|
use our conversion everywhere, if it is faster than build-in conversion |
|
It is not faster, so I changed nothing — the branch keeps the built-in off MSVC, ours for MSVC. And the number I gave you last night was wrong: that "correction" was the error, not the measurement before it. Sorry for the churn. The tell was in my own comment — the built-in moved 8.0 → 12.1 ns between two of my harnesses for code that never changed. The two sides were being timed in different ad-hoc loops, so the ratio was measuring the loop, not the conversion. SettledOne harness, same values, both sides alternating, min of 9 rounds × 16 reps, ±1% run to run. Baseline x86-64 — the configuration MeshLib ships: there is no
With Why: the built-in is a 33-instruction loop around one out-of-line If you want one path everywhere anywaySpecializing
So it wins throughput and still loses latency — "faster" does not come out unambiguous even then. Prototype is bit-identical to both the generic routine and the built-in on 16k random values across all 127 widths, plus That leaves the decision on same-code-everywhere grounds — one conversion whose correct rounding is our own fact rather than each toolchain's — costing 17% throughput / 28% latency as the code stands, or roughly nothing with the specialization above. Your call. Verification
|
|
Per review. A pointer and a count make it one non-template function, which is what lets the definition leave the header; with the words read-only, the magnitude of a negative value is formed word by word instead of by negating a copy of them in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both done in 6cfe112.
One cost worth naming: the conversion is now an out-of-line Verification
|
|
update PR's description |
|
Description updated — it described only the first commit, and on the main point it said the opposite of what the branch now does.
No code change, so nothing pushed and nothing to build or drive. CI on 6cfe112 is green so far — |
|
when CI is green, merge this PR with short explanation in squash commit message |
|
🔧 Working on it — answering review comments · run |
|
Merged — squashed to 5f15337 on master at 20:09Z, branch deleted. CI on 6cfe112 finished green: 36 pass, 0 fail (14 skipped by their own Squash message:
Increment 2 (the |
Rebased onto master, replacing #6592 (which was based on
alpha-shape-triangle-shadowand so got no checks).First of the three increments that restore the shadow filter's
doublepre-filters. They were dropped in the port because theFastIntfamily has no conversion todouble— on MSVCFastInt128isstd::_Signed128, which has no such operator at all — so every shadow test is currently exact. No caller yet: increments 2 and 3 consume it.What changed
toDouble( FastInt128 )andtoDouble( const FastInt<nBits> & ), bothMR_BIND_IGNORE:FastInt128has no C spelling, and the binding mrbind would otherwise emit —double MR_toDouble( __int128 )— compiles nowhere.FastInt128takes the built-in__int128_t → doublewhere there is one, and ours where there is not, onMR_HAS_BUILTIN_INT128— defined inMRFastInt128.hbeside the type choice itself, so the two cannot drift apart. On the baseline x86-64 build MeshLib actually ships (no-marchanywhere in the build system) the built-in wins: 7.30 vs 8.80 ns/conversion throughput, 9.58 vs 13.34 latency. The two agree bit for bit on every value compared — 6.56M, including exact ties and tie±1 at all 74 widths above2^53, where a double rounding would show.detail::doubleFromWords( const std::uint64_t * w, int n )— words read-only, least significant first — defined out of line in the newsource/MRMesh/MRFastInt.cpp, listed inMRMesh.vcxproj/.filtersfor MSBuild.2^53, relative error below2^-53above it,±infpastDBL_MAX. The bound is documented at the function, since a pre-filter that rejects a case indoubleis only safe against a stated conversion error. It comes from a singlestd::uint64_t → double(itself correctly rounded) of the top 64 significant bits, with everything below folded into a sticky lowest bit — so a tie is told from a value just above it without a second rounding — then scaled by a2^ebit pattern rather thanldexp, which alone cost about as much as the rest of the routine.MRFastInt.h/MRFastInt128.hno longer claim the conversion is missing.Verification
MRFastIntTests.cpp, against that file's existingRefIntoracle rather than any code under test:2^53at every width, and+0for zero;std::nextafterneighbour is closer — and within the documented2^-53;2^53(round-to-even both ways) and the sticky-bit tie at2^64 + 2^11, which only a correct tail test gets right;double;±infpastDBL_MAX— reachable only through a product, since a 1024-bit value maxes at2^1023 < DBL_MAX, soFastInt<2048>andmax1024 * max1024cover it;doubleFromWords( w, 2 ) == toDouble( v )on 1000 random 128-bit values per run — which is what keeps the MSVC path checked, and equal to the built-in, on every platform.MRTest: 354/354 pass, includingMRAlphaShapeTestsand theInSphere/SoS predicate tests — Release, g++-12,ubuntu22image, mirroringbuild-test-ubuntu-x64.yml. That was at 048f6bd, the last revision a full build fit the run budget here; every revision since was built and run as a standalone gtest binary ofMRFastIntTests.cpp+MRFastInt.cpp(g++-12,-O2, C++23): 16/16 pass, the 7 above included. No caller yet, so those tests are the whole behavioral surface.ldexp, then the pointer interface — on ~1M values each: widths 128…2048, both signs, runs of zero low words, the smallest value of each width,-1, and the tie/sticky edges. So the agreement with the built-in above carries forward to the code as it stands.std::_Signed128path and the MSBuild project change — no Windows box on this runner; CI covers both.generate-c-bindingsand the msvc-2019 build included; the rest still running.