Skip to content
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

No longer use ffast-math #2960

Closed
wants to merge 1 commit into from
Closed

No longer use ffast-math #2960

wants to merge 1 commit into from

Conversation

@jfix71
Copy link
Contributor

jfix71 commented May 23, 2019

Summary: Regardless of whether we are compiling a model in Release or Debug mode, we should try to generate the same binary. Prior to this PR, for example the GraphOptimizer may have quantized a Constant Tensor payload slightly differently due to ffast-math when in Release vs. Debug.

Test Plan: All tests pass.

Fixes #3856

@jfix71 jfix71 force-pushed the jfix71:remove_fast_math branch from 41aa614 to 6cd1e6b May 23, 2019
Copy link
Contributor

rdzhabarov left a comment

Do we have matching behavior in the fbcode?

@jfix71 jfix71 force-pushed the jfix71:remove_fast_math branch from 6cd1e6b to 2a8e6da May 23, 2019
Copy link

facebook-github-bot left a comment

@jfix71 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jfix71

This comment has been minimized.

Copy link
Contributor Author

jfix71 commented May 23, 2019

Do we have matching behavior in the fbcode?

I will make sure we do when landing this.

@stale

This comment has been minimized.

Copy link

stale bot commented Jun 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale

This comment has been minimized.

Copy link

stale bot commented Jun 22, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale

This comment has been minimized.

Copy link

stale bot commented Jul 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale

This comment has been minimized.

Copy link

stale bot commented Jul 22, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale

This comment has been minimized.

Copy link

stale bot commented Aug 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

rdzhabarov commented Aug 20, 2019

@jfix71 do you want to keep this open? I see there is a fight with the stale bot going on ;)

@stale

This comment has been minimized.

Copy link

stale bot commented Sep 4, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale_will_be_closed label Sep 4, 2019
@jfix71 jfix71 force-pushed the jfix71:remove_fast_math branch from 2a8e6da to 87bcab9 Sep 16, 2019
@stale

This comment has been minimized.

Copy link

stale bot commented Oct 1, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale

This comment has been minimized.

Copy link

stale bot commented Oct 16, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@SplitInfinity

This comment has been minimized.

Copy link
Contributor

SplitInfinity commented Oct 29, 2019

@jfix71

Do you plan to land this?

@stale stale bot removed the stale_will_be_closed label Oct 29, 2019
@stale

This comment has been minimized.

Copy link

stale bot commented Nov 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale

This comment has been minimized.

Copy link

stale bot commented Nov 28, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@bertmaher

This comment has been minimized.

Copy link
Contributor

bertmaher commented Dec 5, 2019

This seems fine let's go ahead and land it before it hits its 1st birthday and we have to take photos of it smashing a cake or something.

@stale stale bot removed the stale_will_be_closed label Dec 5, 2019
@jfix71

This comment has been minimized.

Copy link
Contributor Author

jfix71 commented Dec 5, 2019

Yeah so the blocker for this landing was some issues in implementing it in fbcode correctly. And since then we also added constant folding (run by the Interpreter) which means that the Interpreter should also probably always be built without ffast-math, because otherwise compilation results will still depend on whether it's a Debug or Release build. So this needs to be sorted before landing.

Alternatively we could just disable fast-math entirely...

@bertmaher

This comment has been minimized.

Copy link
Contributor

bertmaher commented Dec 5, 2019

I think we should disable -ffast-math entirely. It's not enabled at all inside FB. The amount of effort that goes into verifying numerical correctness is really big, and -ffast-math puts that entirely at the whims of the compiler in ways that are really hard to predict (e.g., looking at the recently linked issue, it's not even easy to understand that -ffast-math is going to mess that up).

@jfix71 jfix71 force-pushed the jfix71:remove_fast_math branch from 87bcab9 to c4a0b32 Dec 5, 2019
@jfix71 jfix71 changed the title Only use ffast-math when building Backends in Release No longer use ffast-math Dec 5, 2019
Copy link

facebook-github-bot left a comment

@jfix71 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jfix71 jfix71 deleted the jfix71:remove_fast_math branch Dec 5, 2019
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Dec 10, 2019

@jfix71 merged this pull request in 2850714.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.