Skip to content

is_nan / is_finite answer IEEE under fast math (clang 17+ nofpclass, GCC optimize attribute, options fast_math regions) - #3935

Merged
borisbat merged 1 commit into
masterfrom
bbatkin/isnan-fastmath
Sep 4, 2026
Merged

is_nan / is_finite answer IEEE under fast math (clang 17+ nofpclass, GCC optimize attribute, options fast_math regions)#3935
borisbat merged 1 commit into
masterfrom
bbatkin/isnan-fastmath

Conversation

@borisbat

@borisbat borisbat commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Behavior change: is_nan and is_finite now answer IEEE in a runtime or AOT unit built with fast math (-ffast-math, -ffinite-math-only, clang-cl -fp:fast), and inside options fast_math AOT 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) == false and is_finite(inf) == true. The builtins fold under finite-math-only, and clang marks every float parameter nofpclass(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-only its optimize("no-finite-math-only") attribute keeps is_nan right but leaves is_finite(nan) true. And a precise runtime was not safe either once the helpers inline: the AOT emitter wraps every function of an options fast_math program in a GCC optimize("fast-math") region, and an inlined __builtin_isnan folds there.

The four helpers in aot_builtin_math.h are now one implementation on GCC and clang in every build: a volatile copy 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 keeps isnan / isfinite, which it does not fold. The precise-build cost moves from a noinline call 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.das and three cases in tests/daslib/test_toml.das probed is_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), and inf_and_nan.das gains a test of is_nan / is_finite on 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) and tests-cpp/small/test_isnan_fastmath_region.cpp (a precise TU with the checks inside DAS_FAST_MATH_PUSH). Both TUs rename the helpers through #define before the include, because the executable also links libDaScript and the linker keeps one copy per inline name; without the rename the fast-math TU was observed passing against the precise copy.

Validation, claims, ledger

Validation

  • Standalone repro (bit-built nan/inf/finite through the real header), old header vs this one. Old header wrong cells / new header wrong cells, whole TU under fast math: clang-cl 21.1.8 -fp:fast on Windows 6/0, Homebrew clang 22 and Apple clang (arm64) 6/0, clang 18 (WSL x86-64) 2/0 with -ffast-math and 6/0 with -ffinite-math-only, GCC 12 and GCC 13 2/0 (is_finite(nan)), clang 14 0/0, MSVC 19 /fp:fast 0/0. Precise TU with the checks inside the options fast_math region: GCC 12 was 0 wrong on the old header and 2 wrong with __forceinline builtins, GCC 13 6 wrong; 0 wrong with the volatile launder on every compiler above.
  • The in-suite fast-math gate was observed red at the old header and green at this one on the macOS build (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:fast sets 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 Nintendo DAS_FAST_MATH_PUSH is empty and the test is a precise pass.
  • tests/math/inf_and_nan.das and tests/daslib/test_toml.das on the precise local build: interpreter and -jit, 0 skipped.
  • A runtime built -ffast-math (fresh clone in WSL on x86-64, GCC 13 and clang 18 giving identical numbers): with this header inf_and_nan.das 7 passed / 1 skipped and test_toml.das 104 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.das 2 failed. Note for anyone repeating the clang build: the repo's -Werror trips on clang's -Wnan-infinity-disabled in debug_print.h under -ffast-math, which is unrelated to this change; the run silenced that one warning.
  • ctest -L small locally: 115 green with both new TUs; full preflight 22 passed, 0 failed, 1 skipped (md-ascii, no markdown changed).

Claims - stated, not tested

  • Per-query cost in the shipped precise build goes down (a noinline call 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 but is_nan calls, and the alternative (keeping the builtins for precise builds) is the one the options fast_math region rules out.
  • is_nan(x) where x is the result of the region's own arithmetic under options fast_math may still see a folded x (fast math is allowed to fold x / x); the queries are only promised to honor the bits they are given.

Not done

  • The auditor-proposed wording changes to tests-cpp/REVIEW.md (a test whose subject compiles to an unexercised implementation on some lane) and include/daScript/simulate/REVIEW.md (which build flavor judges hot-path cost), and the writing_cpp_tests.md line saying a small test needs no CMake edit. Rule-document edits, left for a separate ruling.
  • A tests-cpp/REVIEW.das gate that every path named in a set_source_files_properties line exists (a moved or renamed fast-math TU would silently build precise).

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings September 4, 2026 14:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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/disfinite in aot_builtin_math.h using 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_finite always 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>
@borisbat
borisbat force-pushed the bbatkin/isnan-fastmath branch from 3b7d67d to 462b820 Compare September 4, 2026 14:24
Copilot AI review requested due to automatic review settings September 4, 2026 14:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread tests-cpp/small/test_isnan_fastmath.cpp
Comment thread tests-cpp/small/test_isnan_fastmath_region.cpp
@borisbat
borisbat merged commit 9dc8f48 into master Sep 4, 2026
36 checks passed
@borisbat
borisbat deleted the bbatkin/isnan-fastmath branch September 4, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants