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

Keep possibly-effectful expressions when optimizing multiplication by zero #956

Merged
merged 2 commits into from Dec 9, 2016

Conversation

Projects
None yet
5 participants
@yallop
Member

yallop commented Dec 8, 2016

Here's a test program:

let () = Printf.printf "%d\n" (0 * (print_endline "erk"; 1))

Output before this PR:

0

Output after this PR:

erk
0
@@ -156,8 +156,7 @@ and mult_power2 c n dbg = lsl_int c (Cconst_int (Misc.log2 n)) dbg
let rec mul_int c1 c2 dbg =
match (c1, c2) with
| (_, Cconst_int 0) | (Cconst_int 0, _) ->
Cconst_int 0
| (c, Cconst_int 0) | (Cconst_int 0, c) -> Csequence (c, Cconst_int 0)

This comment has been minimized.

@nojb

nojb Dec 8, 2016

Contributor

Wouldn't it be better to preserve the old rule, but guarded with when is_pure c ? (The function is_pure needs to be written.)

@nojb

nojb Dec 8, 2016

Contributor

Wouldn't it be better to preserve the old rule, but guarded with when is_pure c ? (The function is_pure needs to be written.)

This comment has been minimized.

@yallop

yallop Dec 8, 2016

Member

I actually wrote it that way first, but it turns out to be a much bigger and more controversial change, and the Csequence style is used elsewhere in this file.

I think it's better to fix this as simply and directly as possible, and consider is_pure as a separate enhancement.

@yallop

yallop Dec 8, 2016

Member

I actually wrote it that way first, but it turns out to be a much bigger and more controversial change, and the Csequence style is used elsewhere in this file.

I think it's better to fix this as simply and directly as possible, and consider is_pure as a separate enhancement.

@mshinwell mshinwell added the bug label Dec 8, 2016

@mshinwell mshinwell added this to the 4.05.0 milestone Dec 8, 2016

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Dec 9, 2016

Contributor

Reviewed and approved.

Contributor

xavierleroy commented Dec 9, 2016

Reviewed and approved.

@xavierleroy xavierleroy added the approved label Dec 9, 2016

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 9, 2016

Contributor
Contributor

shindere commented Dec 9, 2016

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Dec 9, 2016

Member

Shouldn't there be the same kind of code for the constant 1?

The 1 case is fine: simplifying e * 0 to 0 isn't correct if e has effects, but simplifying e * 1 to e doesn't have that problem, since it doesn't discard e.

Member

yallop commented Dec 9, 2016

Shouldn't there be the same kind of code for the constant 1?

The 1 case is fine: simplifying e * 0 to 0 isn't correct if e has effects, but simplifying e * 1 to e doesn't have that problem, since it doesn't discard e.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 9, 2016

Contributor
Contributor

shindere commented Dec 9, 2016

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 9, 2016

Contributor

I had a look through some of the other various arithmetic functions and didn't spot anything else amiss, although these problems can be rather subtle.

Contributor

mshinwell commented Dec 9, 2016

I had a look through some of the other various arithmetic functions and didn't spot anything else amiss, although these problems can be rather subtle.

@mshinwell mshinwell merged commit f3666f2 into ocaml:trunk Dec 9, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 9, 2016

Contributor

(Sorry, I merged this before realising there wasn't a test case. I will ask Jeremy)

Contributor

mshinwell commented Dec 9, 2016

(Sorry, I merged this before realising there wasn't a test case. I will ask Jeremy)

@yallop yallop deleted the yallop:zeromul branch Dec 15, 2016

nojb added a commit to nojb/ocaml that referenced this pull request Dec 29, 2016

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

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