Skip to content

Fix fast_integer_divide for signed numerator with denominator == 1 - #9271

Merged
alexreinking merged 3 commits into
mainfrom
alexreinking/fast-integer-divide-d1-fix
Aug 5, 2026
Merged

Fix fast_integer_divide for signed numerator with denominator == 1#9271
alexreinking merged 3 commits into
mainfrom
alexreinking/fast-integer-divide-d1-fix

Conversation

@alexreinking

@alexreinking alexreinking commented Aug 1, 2026

Copy link
Copy Markdown
Member

The signed round-toward-negative-infinity branch of fast_integer_divide rewrites numerator in place (numerator = xsign ^ numerator) to fold in the sign, but the denominator == 1 special case then returned that bit-flipped value instead of the original. So fast_integer_divide(n, 1) returned ~n for negative signed n (e.g. int8 -128 -> 127, -5 -> 4, 0 -> -1).

Capture the original numerator before the branch and use it in the denominator == 1 select. The unsigned and round-to-zero branches never reassigned numerator, so they are unaffected.

Adds test/correctness/fast_integer_divide.cpp, which compares fast_integer_divide/fast_integer_modulo against floor division over u8/u16/u32/s8/s16/s32; it exercises the signed denominator == 1 case that previously went uncovered.

Breaking changes

fast_integer_divide now returns 0 with a zero denominator. fast_integer_modulo now returns 0 with a 0 modulus.

Checklist

  • Tests added or updated (not required for docs, CI config, or typo fixes)
  • Documentation updated (if public API changed)
  • Python bindings updated (if public API changed)
  • Benchmarks are included here if the change is intended to affect performance.
  • Commits include AI attribution where applicable (see Code of Conduct)

@alexreinking

Copy link
Copy Markdown
Member Author

This was found and fixed by Opus while working on something else.

@alexreinking
alexreinking requested a review from abadams August 1, 2026 06:25
@codecov

codecov Bot commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.26%. Comparing base (52c8d33) to head (1b41862).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/FastIntegerDivide.cpp 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9271      +/-   ##
==========================================
+ Coverage   70.18%   70.26%   +0.07%     
==========================================
  Files         257      257              
  Lines       79105    79109       +4     
  Branches    18954    18954              
==========================================
+ Hits        55518    55583      +65     
+ Misses      17908    17866      -42     
+ Partials     5679     5660      -19     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@abadams

abadams commented Aug 3, 2026

Copy link
Copy Markdown
Member

Please fix test/const_division.cpp to include denominators of 1 (and zero) rather than adding a new test.

alexreinking and others added 3 commits August 4, 2026 16:12
The signed round-toward-negative-infinity branch rewrites `numerator`
in place (numerator = xsign ^ numerator) to fold in the sign, but the
denominator == 1 special case then returned that bit-flipped value
instead of the original. So fast_integer_divide(n, 1) returned ~n for
negative signed n (e.g. int8 -128 -> 127, -5 -> 4, 0 -> -1).

Capture the original numerator before the branch and use it in the
denominator == 1 select. The unsigned and round-to-zero branches never
reassigned numerator, so they are unaffected.

Add test/correctness/fast_integer_divide.cpp, which compares
fast_integer_divide / fast_integer_modulo against floor division over
u8/u16/u32/s8/s16/s32; it exercises the signed denominator == 1 case
that previously went uncovered.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Previously fast_integer_divide (and fast_integer_divide_round_to_zero)
treated a zero denominator as a divide by 256, and fast_integer_modulo
inherited that via numerator - ratio*denominator (using the raw 0, not
256, so it just returned the numerator unchanged). Make both match
Halide's ordinary / and %, which are total functions defined to return
0 for a zero denominator.

Replace test/correctness/fast_integer_divide.cpp, added in c7ec49b to
cover the denominator == 1 bug, with an exhaustive check in
test/performance/const_division.cpp that compares fast_integer_divide/
fast_integer_divide_round_to_zero/fast_integer_modulo against a
reference across the full uint8 denominator space, 0 to 255. This
couldn't be done by simply widening the existing f/g/h sweep, since `f`
unrolls the denominator into a compile-time constant and Halide
disallows a literal-zero constant divisor.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@alexreinking
alexreinking force-pushed the alexreinking/fast-integer-divide-d1-fix branch from 5efb299 to 1b41862 Compare August 4, 2026 20:13
@alexreinking alexreinking added the release_notes For changes that may warrant a note in README for official releases. label Aug 4, 2026
@alexreinking

Copy link
Copy Markdown
Member Author

Adding release_notes for the semantic change here.

@alexreinking
alexreinking merged commit 0dba9f1 into main Aug 5, 2026
27 of 30 checks passed
@alexreinking
alexreinking deleted the alexreinking/fast-integer-divide-d1-fix branch August 5, 2026 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_notes For changes that may warrant a note in README for official releases.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants