Skip to content

[Executorch][Portable] Dont upcast to double for sigmoid #6931

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

kirklandsign
Copy link
Contributor

Pull Request resolved: #6892

Upcasting to double for compute precision may not be aten compliant.

Reason for internal test change:
Apparently running on broadwell CPU vs test runner with Cooper lake gives different results for this change.

Without this change:
Both broadwell and Cooper lake will produce "Once upon a time, there was a little"

With this change:
Broadwell still produces "Once upon a time, there was a little", while Cooperlake produces "Once upon a time, there was a girl".

So one possibility is that that some XNNPACK kernel for Cooper lake is produces slightly different numerical result that propagates through.

Still landing this change since upcasting to double for compute, does not seem necessary.
ghstack-source-id: 253832495
@exported-using-ghexport

Differential Revision: D65928920

Summary

[PLEASE REMOVE] See CONTRIBUTING.md's Pull Requests for ExecuTorch PR guidelines.

[PLEASE REMOVE] If this PR closes an issue, please add a Fixes #<issue-id> line.

[PLEASE REMOVE] If this PR introduces a fix or feature that should be the upcoming release notes, please add a "Release notes: " label. For a list of available release notes labels, check out CONTRIBUTING.md's Pull Requests.

Test plan

[PLEASE REMOVE] How did you test this PR? Please write down any manual commands you used and note down tests that you have written if applicable.

Pull Request resolved: #6892

Upcasting to double for compute precision may not be aten compliant.

Reason for internal test change:
Apparently running on broadwell CPU vs test runner with Cooper lake gives
different results for this change.

Without this change:
Both broadwell and Cooper lake will produce "Once upon a time, there was a
little"

With this change:
Broadwell still produces "Once upon a time, there was a little", while
Cooperlake produces "Once upon a time, there was a girl".

So one possibility is that that some XNNPACK kernel for Cooper lake is produces
slightly different numerical result that propagates through.

Still landing this change since upcasting to double for compute, does not seem
necessary.
ghstack-source-id: 253832495
@exported-using-ghexport

Differential Revision: [D65928920](https://our.internmc.facebook.com/intern/diff/D65928920/)
Copy link

pytorch-bot bot commented Nov 18, 2024

🔗 Helpful Links

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

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

❗ 2 Active SEVs

There are 2 currently active SEVs. If your PR is affected, please view them below:

❌ 4 New Failures

As of commit 23a2b0a with merge base dc41596 (image):

NEW FAILURES - The following jobs have failed:

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

@facebook-github-bot facebook-github-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 Nov 18, 2024
@kirklandsign kirklandsign merged commit c242a59 into main Nov 18, 2024
32 of 35 checks passed
@kirklandsign kirklandsign deleted the gh/kimishpatel/142/orig branch November 18, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants