Keep possibly-effectful expressions when optimizing multiplication by zero#956
Keep possibly-effectful expressions when optimizing multiplication by zero#956mshinwell merged 2 commits intoocaml:trunkfrom
Conversation
| 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) |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
|
Reviewed and approved. |
|
Xavier Leroy (2016/12/09 05:33 -0800):
Reviewed and approved.
Shouldn't there be the same kind of code for the constant 1?
|
The |
|
yallop (2016/12/09 05:54 -0800):
> 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`.
Ah indeed, sorry!
Sometimes, thinking before writing wouldn't hurt... ;-)
|
|
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. |
|
(Sorry, I merged this before realising there wasn't a test case. I will ask Jeremy) |
Here's a test program:
Output before this PR:
Output after this PR: