Keep int64 precision in portable div trunc mode and fmod by usmanmateen · Pull Request #24170 · pytorch/executorch

usmanmateen · 2026-10-27T01:49:21Z

Summary

The portable div.out_mode and div.Scalar_mode_out kernels compute the integer quotient with / and then call std::trunc on it, which converts it to double and back. Integer division already truncates, so std::trunc now only runs for floating-point types. For int64 values above 2^53, div(a, 3, rounding_mode="trunc") gave 1537228672809129216 instead of 1537228672809129302.

fmod.Tensor_out and fmod.Scalar_out switched every integer compute type to Double. Integer types now use % through a new utils::integer_fmod in math_util.h, which returns 0 for a divisor of -1 so the most negative value can't overflow. Float types keep the existing std::fmod path, including the vectorized one for the Scalar variant.

Test plan

Added OpDivOutTest.TruncModeKeepsInt64Precision (Tensor and Scalar divisor) and OpFmodTest.Int64KeepsFullPrecision (Tensor, Scalar, and a divisor of -1 with INT64_MIN). The expected values come from eager PyTorch.

I built portable_kernels_test with CMake (EXECUTORCH_BUILD_TESTS=ON, Release) on Linux x86_64 (WSL2) at 5b14a2f. Before this change both new tests fail; after it, the full portable_kernels_test run passes (1696 tests). clang-format 18.1.3 reports no changes on the changed files.

This PR was authored with AI assistance (Claude Code).

cc @nil-is-all @larryliu0820 @manuelcandales @JakeStevens

Values above 2^53 must come back exact from div with rounding_mode="trunc" (Tensor and Scalar) and from fmod (Tensor and Scalar), and fmod by -1 must give 0 for the most negative int64. See pytorch#24169.

div.out_mode and div.Scalar_mode_out called std::trunc on the integer quotient, which converts it to double and back. Integer division already truncates, so trunc now only applies to floating-point types. fmod.Tensor_out and fmod.Scalar_out computed every integer type as double; integer types now use % through utils::integer_fmod, which returns 0 for a divisor of -1 so the most negative value does not overflow. Floating-point types are unchanged. Fixes pytorch#24169

pytorch-bot · 2026-10-27T01:49:26Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24170

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit cea6e90 with merge base 77f5323 ():

NEW FAILURE - The following job has failed:

pull / nxp-mcuxpresso-test / linux-job (gh)
undefined reference to SYS_INFO_PrintAll'`

This comment was automatically generated by Dr. CI and updates every 15 minutes.

JakeStevens · 2026-10-28T14:36:59Z

@claude please review

claude · 2026-10-28T14:37:41Z

Claude finished @JakeStevens's task in 3m 14s —— View job

Claude finished @JakeStevens's task ——

The int64 fix is right, but there's a regression to fix first: integer fmod.Scalar_out now narrows the scalar after the zero check, so a large scalar can turn into a divisor of 0 and crash. The new INT64_MIN % -1 test cases will also probably crash the ATen test variant.

Detailed review

PR Review: #24170

Summary

The PR keeps int64 precision in portable div (trunc mode) and fmod by computing integers as integers instead of going through double. The integer fmod.Scalar_out path has a new divide-by-zero crash for narrow integer dtypes, and the new fmod test likely crashes in the ATen variant.

Code Quality

kernels/portable/cpu/op_fmod.cpp:124-131 with :150, :156 (major, found by all 3 reviewers, confirmed by reading the code): the scalar is narrowed after the zero check, so a % 0 can run.
- The zero check tests utils::scalar_to<double>(b) == 0.
- For an integer scalar, promote_type_with_scalar keeps the tensor's integer dtype (scalar_utils.h:160-162). The new branch then narrows with utils::scalar_to<CTYPE_COMPUTE>(b), which is a plain static_cast.
- Crash example: fmod(int8_tensor, 256). The check sees 256.0 and passes. val_b becomes (int8_t)256 == 0, and integer_fmod evaluates a % 0, which is UB (SIGFPE on x86). An Int tensor with Scalar(int64_t{1} << 32) does the same.
- Wrong-result example: a value that wraps to -1, like int8 with 255 or Int with 4294967295, silently returns 0.
- This is a regression. Before this PR every integer type was computed in double, so these inputs didn't crash.
- Fix: check the narrowed val_b inside the ET_SWITCH_INT_TYPES lambda (val_b == 0 → ET_KERNEL_CHECK_MSG), or reject scalars that don't fit with check_overflow_scalar_cast (scalar_utils.h).
- div_scalar_mode_out (op_div.cpp:~277) has the same check-then-narrow pattern. That predates this PR, but it's worth fixing in the same pass since the PR edits the lines next to it.
Fix this →
kernels/portable/cpu/util/math_util.h:44-59 (minor): new helper doesn't follow the pattern already in this file.
- remainder_override in the same file is an overload pair covering both integer and float types under one name.
- integer_fmod is integer-only, so fmod needs two dispatch branches per op.
- An fmod_override-style overload pair would match the existing style and collapse the branches.

Testing

kernels/test/op_fmod_test.cpp:51-69 (major, 2 of 3 reviewers; suspected, high confidence): the INT64_MIN / -1 cases probably crash the ATen variant.
- op_fmod_test is built for ["aten", "portable"] (kernels/test/targets.bzl:268).
- As far as we recall, ATen's integer CPU fmod kernel is TORCH_CHECK(d != 0); return x % d; with no -1 guard. If so, both the Tensor row (INT64_MIN, -1) and the Scalar call op_fmod_scalar_out(self, -1, out) would SIGFPE and kill the whole ATen test binary. Nobody checked this against the ATen source or ran it, so treat it as unverified.
- That also conflicts with the description's claim that "expected values come from eager PyTorch".
- Fix: gate those cases with if (SupportedFeatures::get()->is_aten) GTEST_SKIP() (the pattern op_div_test.cpp already uses), or move them into a portable-only test.
Coverage gaps (minor):
- Only Long is tested. The new ET_SWITCH_INT_TYPES branches for Byte, Char, Short and Int never run, including the unsigned case where the -1 guard compiles out. A Char/Byte test with Scalar(256) would have caught the crash above.
- The div-by-zero handling was moved into the new integer Tensor branch of fmod, but there is no ET_EXPECT_KERNEL_FAILURE test for an integer zero divisor on either fmod variant.
kernels/test/op_div_test.cpp:593 (minor): test is in the wrong place.
- TEST_F(OpDivOutTest, TruncModeKeepsInt64Precision) sits in the middle of the OpDivScalarOutTest block.
- It also exercises div.Scalar_mode_out, so splitting the Scalar half into an OpDivScalarOutTest case would match the file's layout.

Recommendation

Request Changes

The integer fmod.Scalar_out divide-by-zero is a new crash on inputs that used to work. The ATen-mode test crash needs to be checked and gated if it's real.

Like ATen, fmod.Scalar_out and div.Scalar_mode_out cast the scalar to the tensor's integer dtype, so 256 is 0 for an int8 tensor and must fail as a division by zero, while 255 (-1) and 257 (1) give ATen's results. Also cover every integer dtype and negative values in fmod, integer division by zero in both fmod variants, and move the Scalar half of the int64 trunc test into OpDivScalarOutTest.

fmod.Scalar_out and div.Scalar_mode_out checked the scalar for zero as a double and then cast it to the compute dtype, so a scalar such as 256 for an int8 tensor reached a % 0 or a / 0. Check the cast value instead. fmod now uses a fmod_override overload pair, like remainder_override, so fmod.Tensor_out needs only one switch.

usmanmateen · 2026-10-28T21:08:47Z

Pushed two commits:

Divide-by-zero crash: fmod.Scalar_out and div.Scalar_mode_out now check the divisor after casting it to the compute dtype, so fmod(int8, 256) and div(int8, 256, rounding_mode=...) fail like ATen's ZeroDivisionError. Previously they computed a % 0, and the new tests hit SIGFPE before the fix. 255 and 257 still wrap to -1 and 1 as int8, same as ATen.
Helper: integer_fmod is now a fmod_override overload pair like remainder_override, so fmod.Tensor_out has one switch.
Tests: added all integer dtypes, negative int8, an integer zero divisor for both fmod variants, and the int8 narrowing cases for fmod and div. The Scalar half of the trunc test moved into OpDivScalarOutTest.
ATen mode: the INT64_MIN % -1 cases are safe there. Eager torch.fmod returns 0 for them on torch 2.14.1 (both the Tensor and Scalar forms), which is where the expected values come from, so no skip is needed.

All 1702 portable kernel tests pass.

With if constexpr, the float instantiations of the lambda do not use div_by_zero_error, which clang reports as an unused lambda capture under -Werror.

JakeStevens

lets fix the comment and then can merge thanks!

It had split the integer and floating-point floor_divide overloads.

nil-is-all · 2026-10-30T20:32:48Z

@usmanmateen updating branch with main should enable running lint checks. Do update and give us a ping

usmanmateen · 2026-10-30T21:03:21Z

@nil-is-all thanks, I've merged main into the branch (d4b3559). It merged cleanly and all 1745 portable kernel tests pass. Ready for lint and CI.

nil-is-all · 2026-10-31T00:23:00Z

@nil-is-all thanks, I've merged main into the branch (d4b3559). It merged cleanly and all 1745 portable kernel tests pass. Ready for lint and CI.

could you try rebasing again? Not sure why it still says branch out of date.

usmanmateen · 2026-10-31T01:12:43Z

@nil-is-all main had moved 3 commits since then, so I've merged it again (5752ff0) and the branch is 0 commits behind now. I merged rather than rebased so Jake's approval and the review history stay attached; happy to rebase if you'd prefer. Portable kernel tests still pass (1745). If main moves again before the checks run, the "Update branch" button on the PR should also work.

JakeStevens · 2026-11-02T20:53:55Z

sorry there was a CI failure over the weekend, update one more time and it should kick off properly

usmanmateen · 2026-11-02T23:24:46Z

@JakeStevens done, merged main again (cea6e90). The portable kernel tests still pass (1785).

### Summary `remainder.Scalar_out` checked the scalar for zero as a `double` and then cast it to the compute dtype, so a scalar such as 256 for an int8 tensor became 0 and reached `a % 0` (SIGFPE on x86). The check now runs on the cast value, so like ATen, 256 raises a division-by-zero error while 255 and 257 wrap to -1 and 1. Empty inputs skip the check and return an empty tensor, also matching ATen. The integer `remainder_override` computed `INT_MIN % -1`, which overflows. Like ATen's `remainder_kernel`, a divisor of -1 now returns 0. These are the same issues #24170 fixes for `fmod` and `div`, found while reviewing that PR. ### Test plan Added `MostNegativeByMinusOneIsZero` (int32 and int64), `TensorIntegerDivisionByZeroFails`, `ScalarIntegerDivisionByZeroFails`, `ScalarIsCastToIntegerDtype`, `ScalarIsCastToUnsignedDtype`, and `EmptyInputWithZeroScalarDivisor` to `op_remainder_test.cpp`; expected values match eager PyTorch 2.14. Built `portable_kernels_test` with CMake (Release, `EXECUTORCH_BUILD_TESTS=ON`) on macOS arm64: all 1743 tests pass. With the kernel's scalar zero check removed, `ScalarIntegerDivisionByZeroFails`, `ScalarIsCastToIntegerDtype`, and `ScalarIsCastToUnsignedDtype` fail. `MostNegativeByMinusOneIsZero` passes on arm64 either way because `sdiv` does not trap; it guards the x86 crash. This PR was authored with AI assistance (Claude Code), with an additional review pass by Codex.

…h#24349) Summary: The Cadence HiFi and Fusion G3 operators are forks of the portable kernels and carry the int64 precision bug from pytorch#24169, which pytorch#24170 fixes in portable. HiFi and G3 `div_out_mode` (and G3 `div_scalar_mode_out`) called `std::trunc` on the integer quotient, rounding int64 values above 2^53 through `double`. Integer division already truncates, so `trunc` now only runs for floating-point types. HiFi `fmod.Tensor_out` and `fmod.Scalar_out` forced every integer compute type to `Double`. Integers are now computed with `%`, returning 0 for a divisor of -1 like ATen. `fmod.Scalar_out` checks for a zero divisor after casting the scalar to the compute dtype, so 256 for an int8 tensor fails instead of reaching `a % 0`; empty inputs skip the check like ATen. The local `fmod_value` helper can switch to `utils::fmod_override` once pytorch#24170 lands. HiFi `fmod.Tensor_out` also had an inverted NNLib fast-path condition: Float inputs always took the `std::fmod` fallback, and every other dtype with rank > 0 was sent to `xa_nn_elm_fmod_f32xf32_f32`, which read integer data as float. The fast path is removed rather than fixed: the NNLib kernel computes `a - trunc(a / b) * b` in float32, which is not exact fmod (1.0f and 0.1f give 0 instead of ~0.1), so enabling it would change today's Float results. Float behavior is unchanged by this PR. Test Plan: I could not build or run the Xtensa targets locally. All three changed files pass a host `clang++ -std=c++17 -fsyntax-only -Wall` check against stub NNLib headers, which instantiates every dtype in the switches. The integer fmod and divisor logic matches the portable kernels tested in pytorch#24170 and the portable remainder fix. There is no existing HiFi fmod test target to extend; relying on Cadence CI for the build. This PR was authored with AI assistance (Claude Code), with an additional review pass by Codex. Reviewed By: rascani Differential Revision: D129098757 Pulled By: JakeStevens

Summary: The Cadence HiFi and Fusion G3 operators are forks of the portable kernels and carry the int64 precision bug from #24169, which #24170 fixes in portable. HiFi and G3 `div_out_mode` (and G3 `div_scalar_mode_out`) called `std::trunc` on the integer quotient, rounding int64 values above 2^53 through `double`. Integer division already truncates, so `trunc` now only runs for floating-point types. HiFi `fmod.Tensor_out` and `fmod.Scalar_out` forced every integer compute type to `Double`. Integers are now computed with `%`, returning 0 for a divisor of -1 like ATen. `fmod.Scalar_out` checks for a zero divisor after casting the scalar to the compute dtype, so 256 for an int8 tensor fails instead of reaching `a % 0`; empty inputs skip the check like ATen. The local `fmod_value` helper can switch to `utils::fmod_override` once #24170 lands. HiFi `fmod.Tensor_out` also had an inverted NNLib fast-path condition: Float inputs always took the `std::fmod` fallback, and every other dtype with rank > 0 was sent to `xa_nn_elm_fmod_f32xf32_f32`, which read integer data as float. The fast path is removed rather than fixed: the NNLib kernel computes `a - trunc(a / b) * b` in float32, which is not exact fmod (1.0f and 0.1f give 0 instead of ~0.1), so enabling it would change today's Float results. Float behavior is unchanged by this PR. Test Plan: I could not build or run the Xtensa targets locally. All three changed files pass a host `clang++ -std=c++17 -fsyntax-only -Wall` check against stub NNLib headers, which instantiates every dtype in the switches. The integer fmod and divisor logic matches the portable kernels tested in #24170 and the portable remainder fix. There is no existing HiFi fmod test target to extend; relying on Cadence CI for the build. This PR was authored with AI assistance (Claude Code), with an additional review pass by Codex. Reviewed By: rascani Pulled By: JakeStevens Differential Revision: D129098757 Pull Request resolved: #24349

usmanmateen added 2 commits October 27, 2026 01:49

Test int64 precision of portable div trunc mode and fmod

0c5bd55

Values above 2^53 must come back exact from div with rounding_mode="trunc" (Tensor and Scalar) and from fmod (Tensor and Scalar), and fmod by -1 must give 0 for the most negative int64. See pytorch#24169.

meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 27, 2026

executorch-triage Bot added the community: contribution PRs coming from community (excluding hardware partners) label Oct 27, 2026

nil-is-all requested a review from JakeStevens October 27, 2026 19:30

nil-is-all assigned JakeStevens Oct 27, 2026

nil-is-all added module: kernels Issues related to kernel libraries and utilities, and code under kernels/ release notes: ops & kernels Changes to the opset and any new / changed kernel implementations labels Oct 27, 2026

usmanmateen added 2 commits October 28, 2026 21:06

usmanmateen added 2 commits October 28, 2026 23:40

Use a runtime integral check in fmod.Tensor_out like floor_divide

10a8f7a

With if constexpr, the float instantiations of the lambda do not use div_by_zero_error, which clang reports as an unused lambda capture under -Werror.

Merge remote-tracking branch 'origin/main' into fix/int64-div-trunc-fmod

48ca0bd

JakeStevens reviewed Oct 30, 2026

View reviewed changes

Comment thread kernels/portable/cpu/util/math_util.h Resolved

JakeStevens approved these changes Oct 30, 2026

View reviewed changes

Move fmod_override next to remainder_override

d91c4b1

It had split the integer and floating-point floor_divide overloads.

This was referenced Oct 30, 2026

Fix integer divisor handling in portable remainder #24348

Merged

Cadence: keep int64 precision in div trunc mode and HiFi fmod (#24349) #24349

Merged

Merge remote-tracking branch 'origin/main' into fix/int64-div-trunc-fmod

d4b3559

Merge remote-tracking branch 'origin/main' into fix/int64-div-trunc-fmod

5752ff0

Merge remote-tracking branch 'origin/main' into fix/int64-div-trunc-fmod

cea6e90

JakeStevens merged commit 186ca41 into pytorch:main Nov 3, 2026

Conversation

usmanmateen commented Oct 27, 2026 • edited by pytorch-bot Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

pytorch-bot Bot commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24170

❌ 1 New Failure

Uh oh!

JakeStevens commented Oct 28, 2026

Uh oh!

claude Bot commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: #24170

Summary

Code Quality

Testing

Recommendation

Uh oh!

usmanmateen commented Oct 28, 2026

Uh oh!

Uh oh!

JakeStevens left a comment

Choose a reason for hiding this comment

Uh oh!

nil-is-all commented Oct 30, 2026

Uh oh!

usmanmateen commented Oct 30, 2026

Uh oh!

nil-is-all commented Oct 31, 2026

Uh oh!

usmanmateen commented Oct 31, 2026

Uh oh!

JakeStevens commented Nov 2, 2026

Uh oh!

usmanmateen commented Nov 2, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

usmanmateen commented Oct 27, 2026 •

edited by pytorch-bot Bot

Loading

pytorch-bot Bot commented Oct 27, 2026 •

edited

Loading

claude Bot commented Oct 28, 2026 •

edited

Loading