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

Avoid checking twice if divisor is zero #702

Merged

Conversation

chambart
Copy link
Contributor

The flambda branch before merging assumed that Pdivint and Pmodint where
already checked when entering Cmmgen. This was not the case anymore
after merging and this change was lost. This fix this overlook by adding
an annotation to the Pdivint and Pmodint primitive telling whether the
division by zero was already checked.

The reason to move the test generation to Closure_conversion in the
flambda branch was to allow the division primitive to be considered as
pure without needing to check for the effective value of the
divisor. This simplified Semantics_of_primitives a lot.

The flambda branch before merging assumed that Pdivint and Pmodint where
already checked when entering Cmmgen. This was not the case anymore
after merging and this change was lost. This fix this overlook by adding
an annotation to the Pdivint and Pmodint primitive telling whether the
division by zero was already checked.

The reason to move the test generation to Closure_conversion in the
flambda branch was to allow the division primitive to be considered as
pure without needing to check for the effective value of the
divisor. This simplified Semantics_of_primitives a lot.
@chambart chambart force-pushed the avoid_double_checking_division_by_zero branch from 46f2450 to 2391b4e Compare August 18, 2016 11:28
@chambart
Copy link
Contributor Author

Also contains a fix for http://caml.inria.fr/mantis/view.php?id=7328

The justification for adding the fix here rather than in a separate patch is that it share a lot of this patch.

@damiendoligez would you still accept such a patch for 4.04 ?

@mshinwell
Copy link
Contributor

I think we should merge this for 4.04 given half of it was actually a merge mistake in 4.03, IIUC.

@bobzhang
Copy link
Member

bobzhang commented Aug 22, 2016

according to the docs:

-unsafe
Turn bound checking off for array and string accesses (the v.(i) and s.[i] constructs). Programs compiled with -unsafe are therefore slightly faster, but unsafe: anything can happen if the program accesses an array or string outside of its bounds.

div by zero should always raise.
Btw, if you want to turn -unsafe on to avoid div by zero check, would you use inline record to make life easier for people who consume lambda? thanks

@chambart
Copy link
Contributor Author

chambart commented Aug 23, 2016

Well the doc was missing something then. It should probably be fixed.
By the way this only apply to native code.

@bobzhang
Copy link
Member

here Lambda.primitive types are changed, it would be nice that we use inline records whenever we use tuple

@chambart
Copy link
Contributor Author

chambart commented Aug 25, 2016

@bobzhang I tried and it don't look too heavy, so here it is.

@mshinwell mshinwell added this to the 4.04 milestone Sep 7, 2016
@mshinwell mshinwell added the bug label Sep 7, 2016
@damiendoligez
Copy link
Member

@chambart OK for 4.04 but what does this have to do with the -unsafe flag?

If you think the doc is missing something, you need to update it in this PR...

@lpw25
Copy link
Contributor

lpw25 commented Oct 5, 2016

This looks correct to me.

@damiendoligez It doesn't have anything to do with the -unsafe flag, but it is adjacent to the code which disables raising an exception for division by zero when using -unsafe. I guess @bobzhang noticed it when looking at the patch and was surprised by it since it is not mentioned in the docs.

@damiendoligez
Copy link
Member

@lpw25 Thanks for the clarification. I see the doc change is not so urgent and it belongs in another PR anyway.

This one seems ready to merge. Any objections?

@mshinwell
Copy link
Contributor

Now this has been read we should be good to go.

@damiendoligez damiendoligez merged commit 575f203 into ocaml:trunk Oct 5, 2016
@gasche
Copy link
Member

gasche commented Nov 3, 2016

@damiendoligez I cannot find a trace of this in 4.04, did we forget to cherry-pick it?

damiendoligez pushed a commit that referenced this pull request Nov 4, 2016
* Avoid checking twice if divisor is zero

The flambda branch before merging assumed that Pdivint and Pmodint where
already checked when entering Cmmgen. This was not the case anymore
after merging and this change was lost. This fix this overlook by adding
an annotation to the Pdivint and Pmodint primitive telling whether the
division by zero was already checked.

The reason to move the test generation to Closure_conversion in the
flambda branch was to allow the division primitive to be considered as
pure without needing to check for the effective value of the
divisor. This simplified Semantics_of_primitives a lot.

* Bigarray div and mod also carry safety information

* Handle bigint div and mod like int div and mod in closure_conversion

* Update Changes

* Test for divisions by zero

* Turn Pdivbint and Pmodbint argument into an inline record
@damiendoligez
Copy link
Member

cherry-picked to 4.04 (commit 0ea09d2)

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017
* Avoid checking twice if divisor is zero

The flambda branch before merging assumed that Pdivint and Pmodint where
already checked when entering Cmmgen. This was not the case anymore
after merging and this change was lost. This fix this overlook by adding
an annotation to the Pdivint and Pmodint primitive telling whether the
division by zero was already checked.

The reason to move the test generation to Closure_conversion in the
flambda branch was to allow the division primitive to be considered as
pure without needing to check for the effective value of the
divisor. This simplified Semantics_of_primitives a lot.

* Bigarray div and mod also carry safety information

* Handle bigint div and mod like int div and mod in closure_conversion

* Update Changes

* Test for divisions by zero

* Turn Pdivbint and Pmodbint argument into an inline record
kayceesrk pushed a commit to ocaml-multicore/ocaml that referenced this pull request Oct 22, 2021
otherlibs: add PR10478 fix back to systhreads
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Sep 21, 2022
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Co-authored-by: Cuihtlauac ALVARADO <cuihtmlauac@tarides.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants