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 removing effectful expressions in Closure, and eliminate more non-effectful ones. #983

Merged
merged 11 commits into from Jan 6, 2017

Conversation

Projects
None yet
3 participants
@alainfrisch
Contributor

alainfrisch commented Dec 21, 2016

When a function that ignores some of its parameters is inlined, the
corresponding arguments are still evaluated if they are not determined
to be "pure". This commit improves this purity criterion by reusing
an existing function that detects more cases. Only the non flambda
pipeline is adapted by this commit, but I guess the same optimization
is already part of flambda (CI will tell!) or would be easily added.

This optimization is triggered on the example discussed on the
caml-list in Dec. 2016 ("Closing the performance gap to C").

[EDIT] This PR evolved from adding a simple optimization to fixing a series of bugs introduced
by badly estimating the effectfulness of primitives in the Closure pass. Fixing these bugs is arguably more important than the new optimization.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

This might be buggy, since is_pure_clambda incorrectly considers Pdivint as pure while it can raise an exception (would need to check if the previous uses of is_pure_clambda are affected).

Contributor

alainfrisch commented Dec 21, 2016

This might be buggy, since is_pure_clambda incorrectly considers Pdivint as pure while it can raise an exception (would need to check if the previous uses of is_pure_clambda are affected).

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

The bug is_pure_clambda is a real one. Currently, the following does not raise when compiled with ocamlopt (and it should):

type t = {x: int; y:int}
let f x = {x; y = x/0}.x
let () = ignore (f 1)
Contributor

alainfrisch commented Dec 21, 2016

The bug is_pure_clambda is a real one. Currently, the following does not raise when compiled with ocamlopt (and it should):

type t = {x: int; y:int}
let f x = {x; y = x/0}.x
let () = ignore (f 1)
@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

Fixed Closure.is_pure_clambda.

Contributor

alainfrisch commented Dec 21, 2016

Fixed Closure.is_pure_clambda.

@alainfrisch alainfrisch changed the title from Eliminate more useless computations when inlining to Eliminate more useless computations when inlining, and fix a bug Dec 21, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 21, 2016

Member

Wouldn't it be interesting to avoid catch-all patterns on instructions or primitive in this function? It would force us to consider each case and could uncover other similar bugs today, or tomorrow when a new instruction is added.

Member

gasche commented Dec 21, 2016

Wouldn't it be interesting to avoid catch-all patterns on instructions or primitive in this function? It would force us to consider each case and could uncover other similar bugs today, or tomorrow when a new instruction is added.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

@gasche Agreed. Note that the dangerous catch-all is on primitives (they are assumed to be pure "by default"), not on expressions. But yes, being explicit would be a good idea in both cases.

Contributor

alainfrisch commented Dec 21, 2016

@gasche Agreed. Note that the dangerous catch-all is on primitives (they are assumed to be pure "by default"), not on expressions. But yes, being explicit would be a good idea in both cases.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

Btw, array accesses look a bit fishy as well. At the level of Clambda, the checkbound is not yet explicit, and is_pure_clambda still assumes loads from array to be pure.

Contributor

alainfrisch commented Dec 21, 2016

Btw, array accesses look a bit fishy as well. At the level of Clambda, the checkbound is not yet explicit, and is_pure_clambda still assumes loads from array to be pure.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

Indeed:

type t = {x: int; y:int}
let x = {x = 1; y = [||].(0)}.x

doesn't raise when compiled with ocamlopt.

Contributor

alainfrisch commented Dec 21, 2016

Indeed:

type t = {x: int; y:int}
let x = {x = 1; y = [||].(0)}.x

doesn't raise when compiled with ocamlopt.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

Also suspicious: Plazyforce, Prevapply, Pdirapply, Pmodint, Pdivbint, Pmodbint, Pstring_load_*, Pbigstring_load_*.

Contributor

alainfrisch commented Dec 21, 2016

Also suspicious: Plazyforce, Prevapply, Pdirapply, Pmodint, Pdivbint, Pmodbint, Pstring_load_*, Pbigstring_load_*.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

On the other hand, I don't see why Pduprecord is considered non pure. Does anyone know?

Contributor

alainfrisch commented Dec 21, 2016

On the other hand, I don't see why Pduprecord is considered non pure. Does anyone know?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

Closure.is_pure is also suspicious...

Contributor

alainfrisch commented Dec 21, 2016

Closure.is_pure is also suspicious...

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 21, 2016

Member

Pduprecord, at least if it corresponds to {x with ...}, may either do a initialization of all fields (pure) or a memcopy followed by field-by-field updates. I suppose people had the latter implementation in mind when marking it impure -- you could argue that it only mutates uniquely-owned memory, but I am not sure how true that is (could GC effects share the value or something?).

Member

gasche commented Dec 21, 2016

Pduprecord, at least if it corresponds to {x with ...}, may either do a initialization of all fields (pure) or a memcopy followed by field-by-field updates. I suppose people had the latter implementation in mind when marking it impure -- you could argue that it only mutates uniquely-owned memory, but I am not sure how true that is (could GC effects share the value or something?).

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

Mmh, I don't see how the "memcopy + updates" version would break invariants expected from pure expressions (in this context, "pure" is used to avoid evaluating an expression when its result is not observed; not e.g. to change the evaluation order (which would require to mark all memory reads as also "impure").

Contributor

alainfrisch commented Dec 21, 2016

Mmh, I don't see how the "memcopy + updates" version would break invariants expected from pure expressions (in this context, "pure" is used to avoid evaluating an expression when its result is not observed; not e.g. to change the evaluation order (which would require to mark all memory reads as also "impure").

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

Ok, I've committed a version with an exhaustive match on primitives. I did not try to take the "unsafe" arguments into account.

Contributor

alainfrisch commented Dec 21, 2016

Ok, I've committed a version with an exhaustive match on primitives. I did not try to take the "unsafe" arguments into account.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 21, 2016

Contributor

Can this code just use Semantics_of_primitives instead, which already has a carefully-checked exhaustive list (not that I promise it has no bugs)?

(We may need a flag to allow it to accept primitives that are illegal in Flambda mode, but that's easy.)

Contributor

mshinwell commented Dec 21, 2016

Can this code just use Semantics_of_primitives instead, which already has a carefully-checked exhaustive list (not that I promise it has no bugs)?

(We may need a flag to allow it to accept primitives that are illegal in Flambda mode, but that's easy.)

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 21, 2016

Contributor

Yes, indeed! I'm a bit reluctant to have Closure depends on middle-end/. Can we merge this module into Lambda? One could imagine that either Simplif or alternative backend a-la Bucklescript use the information as well.

Contributor

alainfrisch commented Dec 21, 2016

Yes, indeed! I'm a bit reluctant to have Closure depends on middle-end/. Can we merge this module into Lambda? One could imagine that either Simplif or alternative backend a-la Bucklescript use the information as well.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 21, 2016

Contributor

I think the middle-end dependency may be there already (for Debuginfo), so I'm not sure we should worry about that. If it really must be moved I'd rather it stayed as its own file and was moved properly with git to avoid introducing any merge conflicts with other branches in progress.

Contributor

mshinwell commented Dec 21, 2016

I think the middle-end dependency may be there already (for Debuginfo), so I'm not sure we should worry about that. If it really must be moved I'd rather it stayed as its own file and was moved properly with git to avoid introducing any merge conflicts with other branches in progress.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 22, 2016

Contributor

@mshinwell

Can you comment on:

  | Prevapply
  | Pdirapply
  | Psequand
  | Psequor ->
    Misc.fatal_errorf "The primitive %a should have been eliminated by the \
        [Closure_conversion] pass."
      Printlambda.primitive prim

?

Aren't Prevapply/Pdirapply instead eliminated by Simplif (also for the non-flambda pipeline)?

About Psequand/Pseqor: would you be ok to support them in Semantics_of_primitive (returning No_effects, No_coeffects) instead of failing as above?

Contributor

alainfrisch commented Dec 22, 2016

@mshinwell

Can you comment on:

  | Prevapply
  | Pdirapply
  | Psequand
  | Psequor ->
    Misc.fatal_errorf "The primitive %a should have been eliminated by the \
        [Closure_conversion] pass."
      Printlambda.primitive prim

?

Aren't Prevapply/Pdirapply instead eliminated by Simplif (also for the non-flambda pipeline)?

About Psequand/Pseqor: would you be ok to support them in Semantics_of_primitive (returning No_effects, No_coeffects) instead of failing as above?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 22, 2016

Contributor

I think the middle-end dependency may be there already (for Debuginfo), so I'm not sure we should worry about that.

The middle-end is not linked in the bytecode compiler, only in tools/objinfo.

Contributor

alainfrisch commented Dec 22, 2016

I think the middle-end dependency may be there already (for Debuginfo), so I'm not sure we should worry about that.

The middle-end is not linked in the bytecode compiler, only in tools/objinfo.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 22, 2016

Contributor

Ok, implemented @mshinwell's suggestion of reusing Semantics_of_primitives (after moving it to bytecomp/). I removed error cases in this function, so that it can be applied in more contexts.

Contributor

alainfrisch commented Dec 22, 2016

Ok, implemented @mshinwell's suggestion of reusing Semantics_of_primitives (after moving it to bytecomp/). I removed error cases in this function, so that it can be applied in more contexts.

@alainfrisch alainfrisch changed the title from Eliminate more useless computations when inlining, and fix a bug to Avoid removing effectful expressions in Closure, and eliminate more non-effectful ones. Dec 22, 2016

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 22, 2016

Contributor

@alainfrisch Can you check that [Flambda_invariants] checks for absence of the primitives for which you have removed the error cases? (And make it check if it doesn't already.)

Contributor

mshinwell commented Dec 22, 2016

@alainfrisch Can you check that [Flambda_invariants] checks for absence of the primitives for which you have removed the error cases? (And make it check if it doesn't already.)

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 22, 2016

Contributor

@mshinwell This is the case for Psequand, Pseqor, Pdirapply, Prevapply, but not Ploc. At least this is implemented in Flambda_invariants.primitive_invariants (but I cannot guarantee this function is applied).

Is it worth adding a case for Ploc?

Contributor

alainfrisch commented Dec 22, 2016

@mshinwell This is the case for Psequand, Pseqor, Pdirapply, Prevapply, but not Ploc. At least this is implemented in Flambda_invariants.primitive_invariants (but I cannot guarantee this function is applied).

Is it worth adding a case for Ploc?

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 22, 2016

Contributor

Yeah, I think we might as well. The more checks the better...

Contributor

mshinwell commented Dec 22, 2016

Yeah, I think we might as well. The more checks the better...

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 22, 2016

Contributor

Ok, done. (I'm not sure, but it seems Plazyforce is also expanded by translcore. And contrary to the error message in flambda_invariants, it seems that Prevapply/Pdirapply are handled by Simplif.)

Contributor

alainfrisch commented Dec 22, 2016

Ok, done. (I'm not sure, but it seems Plazyforce is also expanded by translcore. And contrary to the error message in flambda_invariants, it seems that Prevapply/Pdirapply are handled by Simplif.)

@alainfrisch alainfrisch added the bug label Dec 22, 2016

@alainfrisch alainfrisch added this to the 4.05.0 milestone Dec 22, 2016

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 22, 2016

Contributor

Feel free to fix the message!

Contributor

mshinwell commented Dec 22, 2016

Feel free to fix the message!

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 22, 2016

Contributor

The message might be sign that flambda does some extra useless work; it would be good to check first.

Contributor

alainfrisch commented Dec 22, 2016

The message might be sign that flambda does some extra useless work; it would be good to check first.

@@ -139,6 +139,11 @@ let unbox_minor_words () =
ignore (Gc.minor_words () = 0.)
done
let ignore_useless_args () =
let f x _y = int_of_float (cos x) in

This comment has been minimized.

@mshinwell

mshinwell Dec 28, 2016

Contributor

I think f should have [@@inlined always] to ensure that we're testing what you wanted to test, no?

@mshinwell

mshinwell Dec 28, 2016

Contributor

I think f should have [@@inlined always] to ensure that we're testing what you wanted to test, no?

This comment has been minimized.

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

I've added [@inlined always] to the call site (the attribute on the definition would be @@inline, no?).

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

I've added [@inlined always] to the call site (the attribute on the definition would be @@inline, no?).

This comment has been minimized.

@mshinwell

mshinwell Jan 5, 2017

Contributor

Fine

@mshinwell

mshinwell Jan 5, 2017

Contributor

Fine

| Prevapply
| Pdirapply
| Pdirapply ->

This comment has been minimized.

@mshinwell

mshinwell Dec 28, 2016

Contributor

Please un-indent these three lines to follow the rest of the file...

@mshinwell

mshinwell Dec 28, 2016

Contributor

Please un-indent these three lines to follow the rest of the file...

This comment has been minimized.

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

The indentation is coherent with was ocp-indent does, considering the definitions in the toplevel .ocp-indent. It is also coherent with the style used elsewhere in the compiler (except in newest flambda code, I guess), and I find it more readable than the style used elsewhere in this file. Since the file moves to bytecomp, I'd rather apply a style similar to other files in this directory, i.e. apply ocp-indent on the whole file. Would you oppose to that?

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

The indentation is coherent with was ocp-indent does, considering the definitions in the toplevel .ocp-indent. It is also coherent with the style used elsewhere in the compiler (except in newest flambda code, I guess), and I find it more readable than the style used elsewhere in this file. Since the file moves to bytecomp, I'd rather apply a style similar to other files in this directory, i.e. apply ocp-indent on the whole file. Would you oppose to that?

This comment has been minimized.

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

I've done it (cf last commit), let me know if you are ok.

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

I've done it (cf last commit), let me know if you are ok.

This comment has been minimized.

@mshinwell

mshinwell Jan 5, 2017

Contributor

I will manage (no doubt it will cause a merge conflict somewhere though, but not to worry)

@mshinwell

mshinwell Jan 5, 2017

Contributor

I will manage (no doubt it will cause a merge conflict somewhere though, but not to worry)

Show outdated Hide outdated asmcomp/closure.ml
| Lprim(_, args,_) -> List.for_all is_pure args
| Levent(lam, _ev) -> is_pure lam
| Lprim(p, args,_) -> is_pure_prim p && List.for_all is_pure args
| Levent(lam, _ev) -> is_pure lam (* can this happen in native code? *)

This comment has been minimized.

@mshinwell

mshinwell Dec 28, 2016

Contributor

I think Levent may be present in a very few cases on the gdb branch, but I would have to check. I would just remove this comment---it probably makes things more robust to include this case correctly, anyway.

@mshinwell

mshinwell Dec 28, 2016

Contributor

I think Levent may be present in a very few cases on the gdb branch, but I would have to check. I would just remove this comment---it probably makes things more robust to include this case correctly, anyway.

This comment has been minimized.

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

Ok.

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

Ok.

Show outdated Hide outdated asmcomp/closure.ml
@@ -448,6 +451,7 @@ let simplif_prim_pure fpc p (args, approxs) dbg =
| Pfield n, [ Uprim(Pmakeblock _, ul, _) ], [approx]
when n < List.length ul ->
(List.nth ul n, field_approx n approx)

This comment has been minimized.

@mshinwell

mshinwell Dec 28, 2016

Contributor

Gratuitous whitespace change.

@mshinwell

mshinwell Dec 28, 2016

Contributor

Gratuitous whitespace change.

This comment has been minimized.

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

Thanks, fixed.

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

Thanks, fixed.

Show outdated Hide outdated Changes
@@ -15,6 +15,10 @@ Next version (4.05.0):
and "0 mod <expr>" in the case when <expr> was a non-constant
evaluating to zero (Mark Shinwell)
- Eliminate more useless pure computations when inlining functions

This comment has been minimized.

@mshinwell

mshinwell Dec 28, 2016

Contributor

Reference GPR#983

@mshinwell

mshinwell Dec 28, 2016

Contributor

Reference GPR#983

This comment has been minimized.

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

Ok, and moved to the Bug Fix section.

@alainfrisch

alainfrisch Jan 3, 2017

Contributor

Ok, and moved to the Bug Fix section.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 28, 2016

Contributor

@alainfrisch Please change the Flambda_invariants message for Pdirapply and Prevapply to reference Simplif instead of Closure_conversion; I think they are indeed handled there. I've made some other comments. OK after that.

Contributor

mshinwell commented Dec 28, 2016

@alainfrisch Please change the Flambda_invariants message for Pdirapply and Prevapply to reference Simplif instead of Closure_conversion; I think they are indeed handled there. I've made some other comments. OK after that.

@mshinwell mshinwell self-assigned this Dec 28, 2016

alainfrisch added some commits Dec 21, 2016

Eliminate more useless computations when inlining
When a function that ignores some of its parameters is inlined, the
corresponding arguments are still evaluated if they are not determined
to be "pure".  This commit improves this purity criterion by reusing
an existing function that detects more cases.  Only the non flambda
pipeline is adapted by this commit, but I guess the same optimization
is already part of flambda (CI will tell!) or would be easily added.
@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Jan 5, 2017

Contributor

@alainfrisch I'm not sure about the Ploc case actually. Shouldn't that "have effects"? It's semantics depends on it not being moved, I thought. (If we can't pin the semantics down properly it should maybe be a fatal error instead.)

Contributor

mshinwell commented Jan 5, 2017

@alainfrisch I'm not sure about the Ploc case actually. Shouldn't that "have effects"? It's semantics depends on it not being moved, I thought. (If we can't pin the semantics down properly it should maybe be a fatal error instead.)

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jan 5, 2017

Contributor

Ploc evaluates to a compile-time constant, no? Which effect could it have, and why could it not be moved?

Contributor

alainfrisch commented Jan 5, 2017

Ploc evaluates to a compile-time constant, no? Which effect could it have, and why could it not be moved?

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Jan 5, 2017

Contributor

Well it seems to me that either Ploc has been compiled out, and if we only expect Semantics_of_primitives to be used on code after that compilation, we should have it produce a fatal error if it encounters this primitive.

However (reasonably enough) it seemed you were in favour of having Semantics_of_primitives do the right thing for any Lambda code, not just on that subset that we happen to generate at the moment, and in that world if Ploc exists I thought it would need to be treated as effectful to reflect the fact that if it were moved, it would produce a different result.

Contributor

mshinwell commented Jan 5, 2017

Well it seems to me that either Ploc has been compiled out, and if we only expect Semantics_of_primitives to be used on code after that compilation, we should have it produce a fatal error if it encounters this primitive.

However (reasonably enough) it seemed you were in favour of having Semantics_of_primitives do the right thing for any Lambda code, not just on that subset that we happen to generate at the moment, and in that world if Ploc exists I thought it would need to be treated as effectful to reflect the fact that if it were moved, it would produce a different result.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jan 5, 2017

Contributor

to reflect the fact that if it were moved, it would produce a different result.

I guess that by moving, you mean "changing its source location", while I was rather interpreting moving as changing its position in "the tree" (Parsetree, Typedtree, Lambda, etc).

Currently, Ploc is expanded during the Typedtree->Lambda translation, but it could easily be done later, since the Lprim node also keeps the source location. "Moving" Ploc within Lambda is fine as long as you don't change the Location.t attached to the Lprim node.

Contributor

alainfrisch commented Jan 5, 2017

to reflect the fact that if it were moved, it would produce a different result.

I guess that by moving, you mean "changing its source location", while I was rather interpreting moving as changing its position in "the tree" (Parsetree, Typedtree, Lambda, etc).

Currently, Ploc is expanded during the Typedtree->Lambda translation, but it could easily be done later, since the Lprim node also keeps the source location. "Moving" Ploc within Lambda is fine as long as you don't change the Location.t attached to the Lprim node.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Jan 6, 2017

Contributor

By moving I meant a code motion transformation. However from what you say it sounds like it's ok to do that anyway, as long as you don't change the location. I think this is fine then.

Contributor

mshinwell commented Jan 6, 2017

By moving I meant a code motion transformation. However from what you say it sounds like it's ok to do that anyway, as long as you don't change the location. I think this is fine then.

@mshinwell mshinwell added the approved label Jan 6, 2017

@alainfrisch alainfrisch merged commit b0a4467 into ocaml:trunk Jan 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alainfrisch alainfrisch deleted the alainfrisch:eliminate_useless_computations branch Jan 6, 2017

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

Merge pull request #983 from alainfrisch/eliminate_useless_computations
Avoid removing effectful expressions in Closure, and eliminate more non-effectful ones.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment