Skip to content

Stop g++ merging the two square roots of a point interval - #61

Merged
Lastique merged 2 commits into
boostorg:developfrom
SonnyChecksIn:fix-sqrt-point-merge
Oct 23, 2026
Merged

Lastique merged 2 commits into
boostorg:developfrom
SonnyChecksIn:fix-sqrt-point-merge

Conversation

@SonnyChecksIn

@SonnyChecksIn SonnyChecksIn commented Oct 21, 2026 •

Copy link
Copy Markdown

Fixes #60.

With -frounding-math at -O2 and -O3 on x86-64, GCC can reuse a downward-rounded square root for the upper end of a point interval. The operand is now read through a volatile local inside the existing BOOST_DN/BOOST_UP expressions in rounded_arith_std and rounded_arith_opp. The helper and four operand wrappers are the only header changes, and the helper's generic template is a plain return x;, so user-defined types behave as they do today.

The new C++03 regression test checks both ends of 96 positive point intervals, assuming binary64 and round-to-nearest on entry, using a Dekker split with splitter 2^27+1 and a Sterbenz subtraction instead of std::fma. It uses 14 fixed inputs and 82 values from a 30-bit unsigned-long LCG. An exact-policy negative control is included so that a test which cannot see a narrowed interval is caught; it fails with 92 errors, as it must. The test is registered in test/Jamfile.v2 and test/CMakeLists.txt, which are otherwise unchanged.

Reproduced on one x86-64 Linux host with GCC 13.3.0 and Clang 18.1.3. GCC on develop misses 92 of 96 for both policies at -O2 and -O3, every miss on the upper end; patched misses none. Clang misses none with either header. The four inputs that never miss are the exact squares in the corpus. All 96 builds across C++98, C++03, C++11 and C++17 compile without warnings under -pedantic -Wall -Wextra -frounding-math. A 10-million-pair cross-check, restricted to candidates within two representable steps of the root and exponents from -500 to 500, found no disagreement with fma or with a separately written exact integer reference.

Scope: one host, two compiler versions, finite inputs. No other platform or compiler version was tested. The operand reload stops this merge in these builds; portable dynamic-rounding correctness still depends on compiler support.

@reach2sayan

Copy link
Copy Markdown
Contributor

@SonnyChecksIn Few comments,

Keep the macros BOOST_DN/BOOST_UP already evaluate their argument after the mode change, so the four sqrt bodies can stay as they are with only the operand wrapped: BOOST_DN(sqrt(detail::reload(x))) and BOOST_UP(sqrt(detail::reload(x))). With g++ and clang at -O2/-O3 this compiles to the same object code as your version. Macros are not great. But removing them could be another project.

The test must build as C++03. (again for now). It currently fails with -std=c++03 on g++ and clang: <cstdint>, <cfenv>, std::uint64_t, std::fma and std::fesetround are C++11 (range-for and ULL also warn under -pedantic).

@SonnyChecksIn

Copy link
Copy Markdown
Author

I got you, let me get to work.

SonnyChecksIn pushed a commit to SonnyChecksIn/interval that referenced this pull request Oct 21, 2026
Review feedback on boostorg#61: wrap only the operand in the four sqrt bodies
(BOOST_DN(sqrt(detail::reload_operand(x)))) instead of restructuring
them, and make the regression test compile as C++03 without warnings
under -pedantic. The exact end checks now use a Dekker split instead of
std::fma; no <cstdint>, <cfenv> or fesetround.
SonnyChecksIn pushed a commit to SonnyChecksIn/interval that referenced this pull request Oct 21, 2026
Review feedback on boostorg#61: wrap only the operand in the four sqrt bodies
(BOOST_DN(sqrt(detail::reload_operand(x)))) instead of restructuring
them, and make the regression test compile as C++03 without warnings
under -pedantic. The exact end checks now use a Dekker split instead of
std::fma; no <cstdint>, <cfenv> or fesetround.
@SonnyChecksIn

SonnyChecksIn commented Oct 21, 2026 •

Copy link
Copy Markdown
Author

Thanks for the review. Taking the two points in turn:

  1. Macros kept. The four sqrt bodies use BOOST_DN/BOOST_UP again; only the operand is wrapped with detail::reload_operand(x). In rounded_arith_std, both macros set the mode before evaluating the expression. In rounded_arith_opp, BOOST_DN sets downward mode before evaluation and restores upward mode afterward; BOOST_UP sets no mode at all and relies on the upward-mode invariant established by init() and preserved by the other operations. The reload helper is the only other header addition, and its generic template is a plain return x;, so a user-defined T reaching these wrappers behaves exactly as it does today.

  2. The test is C++03. It uses no <cstdint>, <cfenv>, std::fma, std::fesetround, or range-for; the only includes are <boost/numeric/interval.hpp>, <boost/core/lightweight_test.hpp> and <cstdio>. On the tested binary64 implementation, the Dekker split with splitter 2^27+1 reconstructs r*r as p+e, and Sterbenz makes p-x exact for the candidates these inputs produce. The checks assume round-to-nearest on entry: save_state restores whatever mode the caller had, so it preserves nearest rather than establishing it. Every one of the 96 inputs and its tested candidates was checked to satisfy the factor-two condition, with no overflow or underflow in the intermediate operations or the nonzero residuals. The corpus is 14 fixed values and 82 values from a 30-bit LCG in unsigned long.

Reproduced locally on one x86-64 Linux host, GCC 13.3.0 and Clang 18.1.3, with -pedantic -Wall -Wextra -frounding-math:

Build develop header patched header
g++, default policy, -O2 and -O3 92/96 misses 0/96
g++, rounded_arith_std, -O2 and -O3 92/96 misses 0/96
clang++, default and std policies, -O2 and -O3 0/96 0/96
CONTROL_EXACT_POLICY, either compiler and optimization level fails: 92 errors fails: 92 errors

The control is there to show the test can see a narrowed interval, and it failed in all 32 of its builds, as it has to. The miss count is 92 rather than 96 because four of the 96 inputs (1, 2.25, 4 and 9) are exact squares, where the rounded root is exact and neither end can miss. Every GCC miss in every affected build was on the upper end.

The same four figures held in C++98, C++03, C++11 and C++17: 96 builds, no compiler warnings from any of them. A 10-million-pair random sample was then checked at -O2 and -O3 with both compilers, using candidates within two representable steps of sqrt(x) and binary exponents from -500 to 500. There were no disagreements with fma or with a separately written exact integer comparison. That sample covers only the domain just described, not every pair of doubles.

For both compilers at -O2 and -O3, the macro-wrapped and the earlier restructured version produced byte-identical object code, both for a translation unit exposing the sqrt wrappers alone and for the complete regression test. Both Jam and CMake built and ran the registered test with -frounding-math reaching the compile line.

All of the above is one x86-64 host and these two compiler versions. No other platform, compiler or version was tested, and no attempt was made to find which GCC release first merged these roots. The volatile reload stops the merge in these builds because the two roots now depend on separate volatile loads that cannot be shown to be the same value; that is a property of these compilers in these configurations and not a portable language guarantee of dynamic-rounding correctness.

LG

@Lastique

Lastique commented Oct 22, 2026 •

Copy link
Copy Markdown
Member
  1. I feel the merging can happen with any operation, not just the square root. And on any target, not just x86. The question is whether the CSE pass in the optimizer decides that the computations are equivalent, and presumably it would, since the compiler does not take rounding mode changes into account.
  2. I think, you could avoid reload_operand if you just mark the arguments as const volatile&. This would probably produce a slightly better code since the compiler doesn't have to generate as many stores to the volatile.
  3. I think, instead of using volatiles, on gcc and compatible compilers, it would be better to use an asm statement with an input-output argument through a register. This would avoid the pass through memory and may be enough to convince the compiler that the following computations could have different results. I realize that this would be more complicated since the asm constraints are target-specific, but the performance benefit could be worthwhile.

2 and 3 are just my guesses, I didn't test it. To be clear, I don't require those to be done, just a few ideas of improvement. 1 seems like an undiscovered issue waiting to happen.

@Lastique

Copy link
Copy Markdown
Member

Note that MSVC CI fails on AppVeyor.

@reach2sayan

reach2sayan commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Note that MSVC CI fails on AppVeyor.

@Lastique I believe all these failure are due to another 32 bit msvc bug. That bug possibly applies to other functions as well #25 for eg.

@SonnyChecksIn

SonnyChecksIn commented Oct 22, 2026 •

Copy link
Copy Markdown
Author

Thanks both. The logs for the three failing GitHub jobs show only sqrt_point failures: 16 Windows x86 configurations on msvc-14.3, 16 on msvc-14.5, and 12 on clang-win, in debug and release. All 44 corresponding x64 configurations pass. AppVeyor also has six Windows x86 sqrt_point failures on msvc-14.1, all release, with their x64 counterparts passing. The 32-bit and 64-bit Cygwin jobs pass. Every failing configuration reports inputs=96 misses=92, with equal lower and upper bounds for every printed miss.

I compared the unpatched develop header (ef305d5) with this PR's header (ea825d5) on native Windows x86 MSVC, debug and release, on windows-2022 and windows-2025 runners: https://github.com/SonnyChecksIn/interval/actions/runs/39801096612 . Both headers give the identical inputs=96 misses=92 in all four MSVC jobs, so on MSVC x86 the failure predates this PR. A probe in the same run shows Boost's downward()/upward() do set both the x87 control word and MXCSR, yet std::sqrt returns the same value in both directions there. I have opened an issue with the details: #63. The clang-cl comparison did not build in my script, so clang-win x86 is not yet compared.

I propose keeping sqrt_point. A blanket 32-bit Windows exclusion would remove coverage of a real enclosure failure. Since the failure is now shown on the base for MSVC x86, a narrowly scoped expected-failure registration for those configurations is an option to discuss. It would need to preserve diagnostics and make an unexpected pass visible.

The supplied Linux validation records 52 successful test targets across C++11 and C++17 debug configurations, including two sqrt_point targets.

@Lastique, I agree that the common-subexpression risk can extend beyond sqrt and x86. Those other operations and targets remain unaudited here. The operand reload is a workaround observed to stop the GCC merge in the tested builds; it is not a portable guarantee of dynamic-rounding correctness. I think this PR can stay scoped to the reproduced sqrt case if that limitation is explicit.

Your const volatile& and register-asm suggestions are useful follow-ups. As you noted, they are untested alternatives rather than requirements. Any change to the workaround should come with generated-code, correctness and compatibility checks before making a performance claim.

LG

@Lastique

Copy link
Copy Markdown
Member

I agree we should keep the test. But I cannot merge a PR with a failing CI, so either the PR needs to fix the test on 32-bit x86 as well, or it should document the issue and disable the test for that target. I would prefer the former.

@reach2sayan

Copy link
Copy Markdown
Contributor

@SonnyChecksIn I just made a PR #64 - could you rebase - once that's accepted.
@Lastique That should fix the separate 32 bit issue

@SonnyChecksIn

SonnyChecksIn commented Oct 22, 2026 via email

Copy link
Copy Markdown
Author

@Lastique

Copy link
Copy Markdown
Member

Please, rebase.

With g++ -O2 -frounding-math on x86-64, sqrt_down and sqrt_up in
rounded_arith_opp and rounded_arith_std take the square root of the
same operand, and the optimiser reuses the downward-rounded result for
the upper end (GCC bug 34678). sqrt(interval<double>(3.0)) then returns
a zero-width interval below the true root; 92 of 96 point inputs miss.

Read the operand back through a volatile local after the rounding mode
is set. With GCC 15.2 this stops the reuse: the lower and upper roots
are evaluated separately in every configuration tested.

Refs boostorg#60.
Checks both ends of 96 point square roots exactly, using a Dekker
split of r*r against x (no fma, so it stays C++03-clean). Built with
-DCONTROL_EXACT_POLICY it must fail, which shows the test can see a
narrowed interval. Registered in Jamfile.v2 and CMakeLists.txt.

Refs boostorg#60.
@SonnyChecksIn

Copy link
Copy Markdown
Author

Please, rebase.

Rebased onto develop (be44916), keeping #64's sqrt_rounded dispatch and both sqrt_rounding and sqrt_point registered in Jam and CMake.

The two changes address different failures. #64 changes the 32-bit MSVC square-root implementation to follow the active rounding mode. This change reads each operand through a volatile local inside the existing rounding expressions, preventing the reuse observed in my GCC tests. GCC bug 34678 describes the broader rounding-mode optimization issue.

I tested GCC 15.2 on x86-64 Linux with -frounding-math, C++03/11/17, -O0/-O2/-O3, and SSE or x87 math. At -O2 and -O3 with SSE math, develop misses 92 of 96 point inputs under both the default and rounded_arith_std policies. The resolved change misses 0 of 96 in every non-control configuration I tested. Removing the reload brings the failure back. All 54 exact-policy negative-control runs fail as intended.

The existing sqrt_rounding test passes throughout. The full CMake suite results are 24/24 on develop and 25/25 with the change in each of five tested configurations. The direct builds are warning-free under -pedantic -Wall -Wextra, including C++03; the full suite retains its existing test_float.cpp warnings.

I haven't tested MSVC or clang-cl locally; the AppVeyor build for this PR passed.

And apologies; I have had some breakthroughs on some of my personal projects that took me away from this for a tiny bit.

@codecov

codecov Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.60%. Comparing base (be44916) to head (821a34a).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #61      +/-   ##
===========================================
+ Coverage    82.51%   82.60%   +0.08%     
===========================================
  Files           23       23              
  Lines         1001     1006       +5     
  Branches       410      410              
===========================================
+ Hits           826      831       +5     
  Misses         167      167              
  Partials         8        8              
Files with missing lines Coverage Δ
include/boost/numeric/interval/rounded_arith.hpp 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be44916...821a34a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Lastique
Lastique merged commit 27a6fdf into boostorg:develop Oct 23, 2026
63 checks passed
@Lastique

Copy link
Copy Markdown
Member

Thanks.

@SonnyChecksIn

Copy link
Copy Markdown
Author

Anytime; it was an honor to help. I love hard puzzles.

Would be glad to throw in anytime.

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.

sqrt of a point interval can lose its upper end with g++ -O2 -frounding-math on x86-64 (1.92.0 and develop)

4 participants