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

Incorrect propagation of -0. #6442

Closed
vicuna opened this issue May 24, 2014 · 4 comments
Closed

Incorrect propagation of -0. #6442

vicuna opened this issue May 24, 2014 · 4 comments

Comments

@vicuna
Copy link

vicuna commented May 24, 2014

Original bug ID: 6442
Reporter: @yakobowski
Status: closed (set by @xavierleroy on 2017-02-16T14:16:48Z)
Resolution: fixed
Priority: normal
Severity: major
Target version: 4.02.0+dev
Fixed in version: 4.02.0+dev
Category: back end (clambda to assembly)
Related to: #7042

Bug description

In the attached code, l should be equal to -0. This code compiled with ocamlopt from the trunk results in 0. (ocamlc exhibits no problem.) Option -no-float-const-prop has no effect.

File attachments

@vicuna
Copy link
Author

vicuna commented May 24, 2014

Comment author: @yakobowski

The culprit seems to be an unfortunate interaction between Alain's work on sharing constants, and Xavier's work on floating-point constant propagation. The ~shared argument to Compilenv.new_structured_constant is probably responsible for sharing 0. and -0., since Pervasives.compare does not distinguish them. Implementing a proper comparison function in Compilenv.CstMap may be the solution.

@vicuna
Copy link
Author

vicuna commented May 24, 2014

Comment author: @yakobowski

I'm not OCaml-compiler savvy enough to fix the comparison function, but I can confirm that disabling the sharing of float constants in closure.ml fixes our issue.

@vicuna
Copy link
Author

vicuna commented May 25, 2014

Comment author: @xavierleroy

Hello Boris,

I confirm that the problem is sharing of constants that are equal in the sense of (=). Constant propagation is innocent :-) Will look into it soon.

@vicuna
Copy link
Author

vicuna commented May 25, 2014

Comment author: @xavierleroy

Fixed in 4.02 branch (commit 14916) and on trunk (commit 14917). Patch against 4.02 is attached.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant