-
Notifications
You must be signed in to change notification settings - Fork 63
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
Handle arithmetic overflow in select_addr #570
Handle arithmetic overflow in select_addr #570
Conversation
Just to make sure I understand this PR: this does not prevent overflows, it merely hopes that overflows on the target will be replicated exactly by |
Use of Misc.no_overflow_ was my first version... Nativeint is slightly simpler but if correctness is more obvious without it, I'm happy to change. What I meant in the optimization is to use is_immediate directly before every fold, which obviates the need for overflow checking on amd64 (I'm ignoring crosscompilation for the moment) |
For cross-compilation, it would be Targetint not Int64, right? |
Both would work, since we're in the |
I modified the code to use Misc.no_overflow_*. |
I'm not sure why you chose to use a Cmm test, I can reproduce the problem quite reliably with the following ml file: let[@inline never][@local never] f n =
let n = Int64.of_int n in
let open Int64 in
to_int (add n (of_int Int.min_int))
let _ = Printf.printf "%d\n%!" (f 1) I think adding a simple |
(The use of a |
I replaced the cmm test with your example, it's so much better, thank you! Just for the recorded, the output before this PR was |
Co-authored-by: Xavier Clerc <xclerc@users.noreply.github.com>
Co-authored-by: Xavier Clerc <xclerc@users.noreply.github.com>
64235a382a flambda-backend: Change Float.nan from sNaN to qNaN (ocaml-flambda#466) 14a8e27063 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (ocaml-flambda#569) c3cda96154 flambda-backend: Add two new methods to targetint for dwarf (ocaml-flambda#560) e6f1fed2f5 flambda-backend: Handle arithmetic overflow in select_addr (ocaml-flambda#570) dab7209a12 flambda-backend: Add Target_system to ocaml/utils (ocaml-flambda#542) 82d5044871 flambda-backend: Enhance numbers.ml with more primitive types (ocaml-flambda#544) 216be99334 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (ocaml-flambda#536) 4b56e07c1d flambda-backend: Test naked pointer root handling (ocaml-flambda#550) 40d69cef86 flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (ocaml-flambda#537) f08ae5851c flambda-backend: Implemented inlining history and use it inside inlining reports (ocaml-flambda#365) ac496bf52e flambda-backend: Disable the local keyword in typing (ocaml-flambda#540) 7d46712f7a flambda-backend: Bugfix for Typedtree generation of arrow types (ocaml-flambda#539) 61a7b47773 flambda-backend: Insert missing page table check in roots_nat.c (ocaml-flambda#541) 323bd36d98 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (ocaml-flambda#534) d8956b09e4 flambda-backend: Persistent environment and reproducibility (ocaml-flambda#533) 4a0c89f117 flambda-backend: Revert "Revert bswap PRs (480 and 482)" (ocaml-flambda#506) 7803705828 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (ocaml-flambda#376) 6199db5b26 flambda-backend: Improve unboxing during cmm for Flambda (ocaml-flambda#295) 96b9e1ba6d flambda-backend: Print diagnostics at runtime for Invalid (ocaml-flambda#530) 42ab88e8a1 flambda-backend: Disable bytecode compilers in ocamltest (ocaml-flambda#504) 58c72d5476 flambda-backend: Backport ocaml/ocaml#10595 from upstream/trunk (ocaml-flambda#471) 10105394de flambda-backend: Use C++ name mangling convention (ocaml-flambda#483) 81881bbf88 flambda-backend: Local allocation test no longer relies on lifting (ocaml-flambda#525) f5c47190f6 flambda-backend: Fix an assertion in Closure that breaks probes (ocaml-flambda#505) c2cf2b2a14 flambda-backend: Add some missing command line arguments to ocamlnat (ocaml-flambda#499) git-subtree-dir: ocaml git-subtree-split: 64235a382a0424cced40eed328ddf1dfb9645f87
64235a382a flambda-backend: Change Float.nan from sNaN to qNaN (#466) 14a8e27063 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (#569) c3cda96154 flambda-backend: Add two new methods to targetint for dwarf (#560) e6f1fed2f5 flambda-backend: Handle arithmetic overflow in select_addr (#570) dab7209a12 flambda-backend: Add Target_system to ocaml/utils (#542) 82d5044871 flambda-backend: Enhance numbers.ml with more primitive types (#544) 216be99334 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (#536) 4b56e07c1d flambda-backend: Test naked pointer root handling (#550) 40d69cef86 flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (#537) f08ae5851c flambda-backend: Implemented inlining history and use it inside inlining reports (#365) ac496bf52e flambda-backend: Disable the local keyword in typing (#540) 7d46712f7a flambda-backend: Bugfix for Typedtree generation of arrow types (#539) 61a7b47773 flambda-backend: Insert missing page table check in roots_nat.c (#541) 323bd36d98 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (#534) d8956b09e4 flambda-backend: Persistent environment and reproducibility (#533) 4a0c89f117 flambda-backend: Revert "Revert bswap PRs (480 and 482)" (#506) 7803705828 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (#376) 6199db5b26 flambda-backend: Improve unboxing during cmm for Flambda (#295) 96b9e1ba6d flambda-backend: Print diagnostics at runtime for Invalid (#530) 42ab88e8a1 flambda-backend: Disable bytecode compilers in ocamltest (#504) 58c72d5476 flambda-backend: Backport ocaml/ocaml#10595 from upstream/trunk (#471) 10105394de flambda-backend: Use C++ name mangling convention (#483) 81881bbf88 flambda-backend: Local allocation test no longer relies on lifting (#525) f5c47190f6 flambda-backend: Fix an assertion in Closure that breaks probes (#505) c2cf2b2a14 flambda-backend: Add some missing command line arguments to ocamlnat (#499) git-subtree-dir: ocaml git-subtree-split: 64235a382a0424cced40eed328ddf1dfb9645f87
Selection performs constant folding on memory addresses for amd64. Large constants can be created with flambda2 that cause overflow. This PR uses nativeint instead of int.
It's possible that better code can be generated if we recognize earlier that constants overflow and fold some of them rather than bailing out on the entire expression. I'll add a cmm-based test.