Fix floating-point division normalization - #10216
Conversation
2bd7b94 to
289f4e6
Compare
38976dc to
bf03ab4
Compare
1751180 to
53b73a2
Compare
There was a problem hiding this comment.
Pull request overview
This PR adjusts the fpa2bv_converter floating-point division (fp.div) lowering to bit-vectors to preserve exact denormal leading-zero counts and to widen the signed intermediate exponent representation only when needed for the given (ebits, sbits) format, avoiding leading-zero truncation and exponent wraparound for supported ebits <= sbits formats (Fixes #10175).
Changes:
- Introduce
unpack_with_lz_width(...)to control the bit-width used to represent leading-zero counts during unpacking/normalization. - Update
mk_divto compute a format-dependent exponent width (exp_bits) and choose an appropriatelz_bitsfor exact denormal handling. - Generalize
round(...)to operate correctly with exponents wider than the legacyebits + 2intermediate width, while preserving the previous formula shape for legacy-width callers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/ast/fpa/fpa2bv_converter.h | Adds a private helper declaration to unpack with a configurable leading-zero bit-width. |
| src/ast/fpa/fpa2bv_converter.cpp | Implements division/exponent-width adjustments, adds unpack_with_lz_width, and updates rounding to support wider exponent intermediates. |
|
How do we best validate this change (and changes like these)? |
|
Regression tests mirroring these unit tests have been added (as
The disabled unit tests are pushed to z3test as SMT-LIB2 regressions; each has a matching |
|
Nikolaj Bjorner (@NikolajBjorner), sorry—I may have misunderstood the testing convention and added the regression in z3test#60. Should I also add a |
|
The changes in IMHO, those changes are not right: |
|
Thanks. I may revert that part, but I’d like to first investigate whether a smaller |
Absolutely, it would be awesome if we could close this gap! |
you did the right thing. I didnt' see your PR until now. Merged it. |
|
Can I merge this? |
|
I think this is still being worked on; perhaps mark it as draft for now? |
|
Let me take a quick look..... last time I checked I thought it was good to go for review |
|
Christoph M. Wintersteiger (@wintersteiger), I think this is ready for another look now—could you re-review when you have a chance? |
|
Christoph M. Wintersteiger (@wintersteiger) - thanks for reviewing! |
|
Thanks for the review. Marking this draft for now while I think this through, will get back to you |
06f458c to
af856fe
Compare
|
thanks for the update |
|
Christoph M. Wintersteiger (@wintersteiger) - appropriate to merge at this point? |
|
Fairly indirect and a poor-man's one-shot prompt investigation. |
|
I haven't had any time too look at this yet, but from a cursory glance I'll have to go through the details of the division changes and I'm still unhappy with the changes to Nikolaj Bjorner (@NikolajBjorner): careful with IEEE 754: this PR is specifically about very weird formats, e.g. tiny width, or ebits > sbits, for which 754-assumptions and properties don't necessarily apply. |
|
|
||
| expr_ref lz(m); | ||
| mk_leading_zeros(sig, ebits+2, lz); // CMW: is this always large enough? | ||
| // Keep round's established ebits+2 leading-zero workspace. Division |
There was a problem hiding this comment.
Unnecessary comment change; no need to comment on what you're keeping.
| unsigned sigma_size = ebits + 2; | ||
|
|
||
| expr_ref sigma_neg(m), sigma_cap(m), sigma_neg_capped(m), sigma_lt_zero(m), sig_ext(m), | ||
| // The exponent workspace is not necessarily wide enough to represent the |
There was a problem hiding this comment.
What's a "workspace"?
The exponent is always wide enough by definition, because we can't change the output width and the additional two bits in the input are enough to make any rounding decision. Is there an example of an operation different from division that requires these changes?
| // would turn a positive sigma into a small unsigned count instead of | ||
| // preserving its negative two's-complement value for the comparison. | ||
| sigma_ext = m_bv_util.mk_sign_extend(sigma_count_size - sigma_size, sigma); | ||
| sigma_neg_ext = m_bv_util.mk_bv_neg(sigma_ext); |
There was a problem hiding this comment.
Is this about the potential corner-case during negation of sigma? I think that was covered before, but it's of course possible that I'm wrong. If so however, that would be fixed by making sigma one bit wider instead of this complicated construction here. I would like to see a concrete example of this though, ideally without involving division.
Summary
Preserve exact denormal leading-zero counts in symbolic
fp.divand support validformats with
ebits > sbits. Division owns the wider leading-zero and exponentworkspaces, handles exceptional exponents before calling the operation-independent
rounder, and preserves sticky information through deep underflow.
The shared
roundcontract remainssig = sbits + 4andexp = ebits + 2. Its genericshift counts retain their exact bounded values without widening the shifted data path.
Deep underflow is rounded locally with sticky preservation.
Fixes #10175.
Regression coverage: Z3Prover/z3test#60 and Z3Prover/z3test#63.
Testing
4a15d7e9b, directly atop this PR head, records theindependent oracle, coverage, expected output, replay instructions, and hashes.
python3 proofs/verify_ebits_gt_sbits.py | diff -u proofs/verify_ebits_gt_sbits.expected.json -python3 proofs/verify_fp_div_semantics.py | diff -u proofs/verify_fp_div_semantics.expected.json -sha256sum -c proofs/SHA256SUMSf42bacbf1combines this source head51b80b21awith z3test head
9593fb035: build/test(14/14 jobs) and OCaml bindings
(2/2 jobs), both successful.
changes only the shift-cap width; the dedicated FP(2,16) regression fails when 18
wraps to 2.
uses the unpatched PR base with the current z3test head; all 24 isolated wide cases
fail in each of its eight regression configurations.
lightweight Python proofs above.