is_nan / is_finite answer IEEE under fast math (clang 17+ nofpclass, GCC optimize attribute, options fast_math regions) - #3935
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The change is well-scoped, addresses a concrete miscompile class across compilers, and adds targeted daScript + C++ tests (including fast-math TU and fast-math region coverage) to prevent regressions.
Pull request overview
This PR fixes is_nan / is_finite correctness under fast-math (e.g., -ffast-math, -ffinite-math-only, clang-cl /fp:fast) by replacing compiler-builtin queries with an implementation that is robust to finite-math assumptions, including inside AOT fast-math regions (options fast_math).
Changes:
- Re-implement
fisnan/disnan/fisfinite/disfiniteinaot_builtin_math.husing a volatile “launder” plus exponent-bit integer tests (avoids folding under fast-math/nofpclass). - Adjust daScript tests to skip only when NaN comparisons fold, and add a never-skip bit-pattern test that
is_nan/is_finitealways behave IEEE. - Add two C++ “small” doctest TUs to validate behavior both in a fast-math-compiled TU and inside
DAS_FAST_MATH_PUSH/POP, plus CMake wiring to compile the fast-math TU with the appropriate flag.
File summaries
| File | Description |
|---|---|
| tests/math/inf_and_nan.das | Switches host probe to NaN-compare folding and adds a never-skip IEEE classification test using bit-built values. |
| tests/daslib/test_toml.das | Updates host probe logic to depend only on NaN-compare folding (not is_nan/is_finite), matching new semantics. |
| tests-cpp/small/test_isnan_fastmath.cpp | New doctest TU compiled under fast-math to ensure the header helpers still return IEEE results. |
| tests-cpp/small/test_isnan_fastmath_region.cpp | New doctest TU validating IEEE results inside the per-function fast-math region macros. |
| tests-cpp/CMakeLists.txt | Adds per-source fast-math compile flag for the fast-math TU. |
| include/daScript/simulate/aot.h | Comment update clarifying the intent/coverage of DAS_FAST_MATH_PUSH/POP. |
| include/daScript/simulate/aot_builtin_math.h | Replaces builtin-based isnan/isfinite helpers with volatile+bit-test implementation for GCC/clang. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…a finite-math build (clang 17+ marks every float parameter nofpclass, which no pragma lifts; GCC's optimize attribute leaves is_finite(nan) true) and an inlined builtin folds inside the options fast_math region aot.h emits, so on GCC and clang the four helpers are one implementation in every build - a volatile copy plus an integer exponent test; the tests that skipped on such a host now hold the two queries to it, and two tests-cpp TUs (one built fast-math, one precise with the checks inside the region) gate it per PR Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
3b7d67d to
462b820
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The newly added C++ tests misuse doctest INFO(...) (comma operator), so the diagnostic context is lost and should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
Behavior change:
is_nanandis_finitenow answer IEEE in a runtime or AOT unit built with fast math (-ffast-math,-ffinite-math-only, clang-cl-fp:fast), and insideoptions fast_mathAOT functions. No rebuild is required beyond picking up the header.A host that builds daslang with clang 17 or newer and fast math got
is_nan(nan) == falseandis_finite(inf) == true. The builtins fold under finite-math-only, and clang marks every float parameternofpclass(nan inf), so even a plain exponent test on the argument folds;float_control(precise, on)restores NaN handling for a function body but cannot lift the attribute from a parameter, which is why the old clang 17-18 special case did not help on 19 and later. GCC was also wrong: under-ffinite-math-onlyitsoptimize("no-finite-math-only")attribute keepsis_nanright but leavesis_finite(nan)true. And a precise runtime was not safe either once the helpers inline: the AOT emitter wraps every function of anoptions fast_mathprogram in a GCCoptimize("fast-math")region, and an inlined__builtin_isnanfolds there.The four helpers in
aot_builtin_math.hare now one implementation on GCC and clang in every build: avolatilecopy of the argument, then an integer test of the exponent field. The copy launders whatever the compiler believes about the value; the test is integer math, which fast math does not touch. MSVC keepsisnan/isfinite, which it does not fold. The precise-build cost moves from anoinlinecall per query to an inlined store, load, and two integer ops with no call, so the shipped build gets cheaper, not dearer.Two tests skipped on exactly this bug:
tests/math/inf_and_nan.dasand three cases intests/daslib/test_toml.dasprobedis_finite(inf) || !is_nan(nan)and called a hit "the host's own configuration". The probe is now a NaN compare on bit-built values (the compare really does fold on such a host; the two queries must not), andinf_and_nan.dasgains a test ofis_nan/is_finiteon bit-built values that never skips.Where to look:
include/daScript/simulate/aot_builtin_math.h(the four helpers),tests-cpp/small/test_isnan_fastmath.cpp(a TU compiled per-source with-ffast-math//fp:fast) andtests-cpp/small/test_isnan_fastmath_region.cpp(a precise TU with the checks insideDAS_FAST_MATH_PUSH). Both TUs rename the helpers through#definebefore the include, because the executable also links libDaScript and the linker keeps one copy perinlinename; without the rename the fast-math TU was observed passing against the precise copy.Validation, claims, ledger
Validation
-fp:faston Windows 6/0, Homebrew clang 22 and Apple clang (arm64) 6/0, clang 18 (WSL x86-64) 2/0 with-ffast-mathand 6/0 with-ffinite-math-only, GCC 12 and GCC 13 2/0 (is_finite(nan)), clang 14 0/0, MSVC 19/fp:fast0/0. Precise TU with the checks inside theoptions fast_mathregion: GCC 12 was 0 wrong on the old header and 2 wrong with__forceinlinebuiltins, GCC 13 6 wrong; 0 wrong with the volatile launder on every compiler above.bin/tests-cpp-small --test-case="*fast-math*"). Real lanes for it: Linux GCC and the Linux/macOS clang lanes,ctest -L small. On the Windows MSVC lane/fp:fastsets no finite-math assumption, so that cell passes without exercising anything; the TU#errors if built without the flag, so losing the per-source flag is loud. The region TU's real lane is Linux GCC (and x86-64 clang); on arm64 clang, e2k and NintendoDAS_FAST_MATH_PUSHis empty and the test is a precise pass.tests/math/inf_and_nan.dasandtests/daslib/test_toml.dason the precise local build: interpreter and-jit, 0 skipped.-ffast-math(fresh clone in WSL on x86-64, GCC 13 and clang 18 giving identical numbers): with this headerinf_and_nan.das7 passed / 1 skipped andtest_toml.das104 passed / 3 skipped, the skips being exactly the four compare-guarded tests, so the new probe fires on a real finite-math host and the never-skipping test passes there. Same build with master's header:inf_and_nan.das2 failed. Note for anyone repeating the clang build: the repo's-Werrortrips on clang's-Wnan-infinity-disabledindebug_print.hunder-ffast-math, which is unrelated to this change; the run silenced that one warning.ctest -L smalllocally: 115 green with both new TUs; full preflight 22 passed, 0 failed, 1 skipped (md-ascii, no markdown changed).Claims - stated, not tested
noinlinecall replaced by an inlined store/load and two integer ops). Not measured; a break would look like a regression on an AOT loop that is nothing butis_nancalls, and the alternative (keeping the builtins for precise builds) is the one theoptions fast_mathregion rules out.is_nan(x)wherexis the result of the region's own arithmetic underoptions fast_mathmay still see a foldedx(fast math is allowed to foldx / x); the queries are only promised to honor the bits they are given.Not done
tests-cpp/REVIEW.md(a test whose subject compiles to an unexercised implementation on some lane) andinclude/daScript/simulate/REVIEW.md(which build flavor judges hot-path cost), and thewriting_cpp_tests.mdline saying a small test needs no CMake edit. Rule-document edits, left for a separate ruling.tests-cpp/REVIEW.dasgate that every path named in aset_source_files_propertiesline exists (a moved or renamed fast-math TU would silently build precise).🤖 Generated with Claude Code