Skip to content

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

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 2 commits into from
Nov 16, 2024

Conversation

kimishpatel
Copy link
Contributor

@kimishpatel kimishpatel commented Nov 15, 2024

Stack from ghstack (oldest at bottom):

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.

Differential Revision: D65928920

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.

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

[ghstack-poisoned]
Copy link

pytorch-bot bot commented Nov 15, 2024

🔗 Helpful Links

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

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

❗ 1 Active SEVs

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit 8eabd3e with merge base dc41596 (image):

FLAKY - The following job failed but was likely due to flakiness present on trunk:

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 15, 2024
kimishpatel added a commit that referenced this pull request Nov 15, 2024
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.

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

ghstack-source-id: 253799652
Pull Request resolved: #6892
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65928920

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.

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

[ghstack-poisoned]
kimishpatel added a commit that referenced this pull request Nov 15, 2024
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/)
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65928920

@facebook-github-bot facebook-github-bot merged commit 6968d01 into gh/kimishpatel/142/base Nov 16, 2024
38 of 41 checks passed
@facebook-github-bot facebook-github-bot deleted the gh/kimishpatel/142/head branch November 16, 2024 06:36
kirklandsign added a commit that referenced this pull request Nov 18, 2024
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/)

Co-authored-by: Kimish Patel <[email protected]>
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. fb-exported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants