Repository navigation
Cadence: keep int64 precision in div trunc mode and HiFi fmod - #24349
Open
JakeStevens wants to merge 2 commits into
Open
JakeStevens wants to merge 2 commits into
JakeStevens wants to merge 2 commits into
Conversation
The HiFi and Fusion G3 div_out_mode (and G3 div_scalar_mode_out) kernels called std::trunc on the integer quotient, which rounds int64 values above 2^53 through double. Integer division already truncates, so trunc now only runs for floating-point types. This mirrors the portable fix for pytorch#24169. 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. HiFi fmod.Tensor_out also had an inverted NNLib fast-path condition: Float inputs always took the std::fmod fallback, and every other dtype was sent to xa_nn_elm_fmod_f32xf32_f32, which read integer data as float. The fast path is removed rather than fixed, because the NNLib kernel computes a - trunc(a / b) * b in float32, which is not exact fmod (1.0f and 0.1f give 0) and would change today's Float results. This change was authored with AI assistance (Claude Code).
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24349
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit 1ea7b32 with merge base cb3f24e ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Contributor
|
Probably worth importing internally for verification. |
Contributor
|
@mcremon-meta has imported this pull request. If you are a Meta employee, you can view this in D129098757. |
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 G3div_scalar_mode_out) calledstd::truncon the integer quotient, rounding int64 values above 2^53 throughdouble. Integer division already truncates, sotruncnow only runs for floating-point types.HiFi
fmod.Tensor_outandfmod.Scalar_outforced every integer compute type toDouble. Integers are now computed with%, returning 0 for a divisor of -1 like ATen.fmod.Scalar_outchecks for a zero divisor after casting the scalar to the compute dtype, so 256 for an int8 tensor fails instead of reachinga % 0; empty inputs skip the check like ATen. The localfmod_valuehelper can switch toutils::fmod_overrideonce #24170 lands.HiFi
fmod.Tensor_outalso had an inverted NNLib fast-path condition: Float inputs always took thestd::fmodfallback, and every other dtype with rank > 0 was sent toxa_nn_elm_fmod_f32xf32_f32, which read integer data as float. The fast path is removed rather than fixed: the NNLib kernel computesa - trunc(a / b) * bin 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 -Wallcheck 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.