Fix for PR#7042: CSE confuses +0.0 and -0.0#295
Conversation
The fix consists in representing float literals by their bit patterns (int64) in the Mach and Linear intermediate languages. A regression test was added to the test suite.
|
Wouldn't it make sense to modify Pervasives.compare in order to distinguish -0. and +0. ? I assume that people using e.g. Hashtbl to memoize pure functions (on immutable structures containing floats) expect the language semantics to be invariant when arguments are replaced by equal values (w.r.t. Pervasives.compare). |
|
The code seems ok. |
|
@alainfrisch : having Pervasives.compare 0.0 (-0.0) <> 0 would cause more problems than it fixes, I'm afraid. In your hash table example, I expect that keys that compare equal with (=) map to identical values, which would no longer be the case with your proposal. |
|
@xavierleroy: it's just weird not to have an equality operator easily available on floats which is compatible with basic operations on floats (i.e. equal arguments map to equal results). How can we implement memoization for, say, functions (float -> float) or (float -> float -> float)? |
|
In mantis it is PR 7042 not PR 7024. Shouldn't the modification be propagated in all the compiler in ( |
|
As I wrote on Mantis, I think the flambda branch needs to be careful with this as well. |
xavierleroy
changed the title
|
@bobot: sorry for the dyslexia; I fixed the title of this PR. @bobot @mshinwell: Yes, I thought about propagating this change "upwards" in other intermediate representations. For clambda (the Uconst_float constructor), it would introduce some extra code because the closure conversion pass performs FP operations over arguments of Uconst_float constructors, as part of FP constant propagation. Nothing dramatic, but it made me pause. |
|
The consensus at the developer meeting was to merge this patch. There is interest in propagating the change of representation to later passes of the compiler. The flambda people were interested in having the relevant operations (float operations on the Int64.t representations) be put in an auxiliary module, that they could use in flambda as well. |
|
Merged manually, commit [trunk 6cd8656], so as to fix the wrong PR number. |
|
In the near future, I'll try pushing the float -> int64 change to higher intermediate languages and submit this as a separate PR. |
* Remember continuations that are applied with trap actions * Record knowledge about trap uses in cont binders and use it in Un_cps
64235a3 flambda-backend: Change Float.nan from sNaN to qNaN (ocaml#466) 14a8e27 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (ocaml#569) c3cda96 flambda-backend: Add two new methods to targetint for dwarf (ocaml#560) e6f1fed flambda-backend: Handle arithmetic overflow in select_addr (ocaml#570) dab7209 flambda-backend: Add Target_system to ocaml/utils (ocaml#542) 82d5044 flambda-backend: Enhance numbers.ml with more primitive types (ocaml#544) 216be99 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (ocaml#536) 4b56e07 flambda-backend: Test naked pointer root handling (ocaml#550) 40d69ce flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (ocaml#537) f08ae58 flambda-backend: Implemented inlining history and use it inside inlining reports (ocaml#365) ac496bf flambda-backend: Disable the local keyword in typing (ocaml#540) 7d46712 flambda-backend: Bugfix for Typedtree generation of arrow types (ocaml#539) 61a7b47 flambda-backend: Insert missing page table check in roots_nat.c (ocaml#541) 323bd36 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (ocaml#534) d8956b0 flambda-backend: Persistent environment and reproducibility (ocaml#533) 4a0c89f flambda-backend: Revert "Revert bswap PRs (480 and 482)" (ocaml#506) 7803705 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (ocaml#376) 6199db5 flambda-backend: Improve unboxing during cmm for Flambda (ocaml#295) 96b9e1b flambda-backend: Print diagnostics at runtime for Invalid (ocaml#530) 42ab88e flambda-backend: Disable bytecode compilers in ocamltest (ocaml#504) 58c72d5 flambda-backend: Backport ocaml#10595 from upstream/trunk (ocaml#471) 1010539 flambda-backend: Use C++ name mangling convention (ocaml#483) 81881bb flambda-backend: Local allocation test no longer relies on lifting (ocaml#525) f5c4719 flambda-backend: Fix an assertion in Closure that breaks probes (ocaml#505) c2cf2b2 flambda-backend: Add some missing command line arguments to ocamlnat (ocaml#499) git-subtree-dir: ocaml git-subtree-split: 64235a3
* based on existing containers, define a set of reusable containers for the main area as well as individual sections
Co-authored-by: Mirza Babar <[email protected]>
6 participants
The fix consists in representing float literals by their bit patterns (int64) in the Mach and Linear intermediate languages. Conversion from float to int64 takes place during instruction selection. Since +0.0 and -0.0 have different bit patterns, they can no longer be identified by generic equality.
I'm submitting this patch for review because I think all changes in intermediate languages should be reviewed.
This change impacts every emitter for every supported platform, current or future. However, the impact is limited and positive (removes a float->int64 conversion that was present in every code emitter).