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

wrong optimization of 1 mod n #6879

Closed
vicuna opened this issue May 26, 2015 · 6 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented May 26, 2015

Original bug ID: 6879
Reporter: filliatr
Assigned to: @mshinwell
Status: closed (set by @xavierleroy on 2017-02-16T14:14:13Z)
Resolution: fixed
Priority: normal
Severity: major
Target version: 4.02.2+dev / +rc1
Fixed in version: 4.02.2+dev / +rc1
Category: back end (clambda to assembly)
Related to: #6749

Bug description

the following code is wrongly optimized by ocamlopt:

let f n = assert (1 mod n = 0)
let () = f 1

Steps to reproduce

ocamlopt test.ml && ./a.out

Additional information

does not show up with ocaml / ocamlc

note that (1 mod 1) is correctly evaluated to 0 by the compiler, hence the
need for a variable n in the code above

@vicuna

This comment has been minimized.

Copy link
Author

commented May 27, 2015

Comment author: @gasche

Luckily, this embarrassing bug was already reported. The fix will be part of the next minor release (4.02.2), due next month.

@vicuna

This comment has been minimized.

Copy link
Author

commented May 27, 2015

Comment author: filliatr

I don't think it is fixed. I just tried with
https://github.com/ocaml/ocaml/tree/4.02
and the bug is still there (with the test I gave above for instance).

The problem is related to these lines in asmcomp/cmmgen.ml:

| (c1, Cconst_int (1 | (-1))) ->
Csequence(c1, Cconst_int 0)
| (Cconst_int(0 | 1 | (-1)) as c1, c2) ->
Csequence(c2, c1)

Here, the divisor is not a constant (but a variable n equal to 1), so the first pattern does not apply. Then we switch to the second one and incorrectly return 1 as the value for 1 mod n, which should be 0 instead.

@vicuna

This comment has been minimized.

Copy link
Author

commented May 27, 2015

Comment author: filliatr

This is not fixed (see the note I just added).

@vicuna

This comment has been minimized.

Copy link
Author

commented May 27, 2015

Comment author: @gasche

Damn, you are correct -- I was too hasty and tested in the toplevel...

Thanks for the persistence!

@vicuna

This comment has been minimized.

Copy link
Author

commented May 27, 2015

Comment author: @mshinwell

Damnit. I will look at it again with Jeremie

@vicuna

This comment has been minimized.

Copy link
Author

commented May 27, 2015

Comment author: @mshinwell

Fixed for 4.02.2

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