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

Fix #8558: use Strict bindings when simplifying Lstaticraise #8559

Merged
merged 2 commits into from Apr 1, 2019

Conversation

Projects
None yet
3 participants
@alainfrisch
Copy link
Contributor

commented Mar 29, 2019

See discussion on #8558.

When eliminating Lstaticraise in Simpl.simplify_exits, arguments were always bound as Alias. But nothing really prevented those argument to have side-effects. This could be the case for e.g. the use of Lstaticraise for compiling exception cases in pattern matching, although in that case those staticraise would cross a try...with and would not be simplified away. Now, with the new simplification pass that eliminates local functions, one could have arguments with side-effects for Lstaticraise which will be eliminated by simplify_exits, and so we need to be more careful before using Alias.

@alainfrisch alainfrisch changed the title Fix #8858: do not blindly use Alias binding Fix #8558: do not blindly use Alias binding Mar 29, 2019

@alainfrisch alainfrisch force-pushed the alainfrisch:fix_8558 branch from cbd2deb to 31d4132 Mar 29, 2019

@alainfrisch alainfrisch added this to the 4.08 milestone Mar 29, 2019

@alainfrisch alainfrisch added the bug label Mar 29, 2019

@xavierleroy
Copy link
Contributor

left a comment

Too complicated for my taste.

Show resolved Hide resolved bytecomp/simplif.ml Outdated

@alainfrisch alainfrisch changed the title Fix #8558: do not blindly use Alias binding Fix #8558: use Strict bindings when simplifying Lstaticraise Mar 29, 2019

@alainfrisch alainfrisch force-pushed the alainfrisch:fix_8558 branch from be57902 to 918a842 Mar 29, 2019

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Too complicated for my taste.

I've pushed a simpler version.

@xavierleroy
Copy link
Contributor

left a comment

Thank you very much! My taste for simple fixes is entirely satisfied :-) I promise we'll soon discuss more general Lambda simplifications that would recover whatever efficiency we may be losing here by doing the simple thing.

Would you please add a Changes entry? (in the 4.08 section).

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Concerning the Changes entry: I may be confused. If the issue was already in 4.07 we need one. If the issue was introduced after 4.07 we do not.

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

In this case it is fairly common to add the new issue number to the entry of the initial PR.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

Ok, apologies, I did not realize that the usual optimization in the Alias case (1 occurrence) was also disabled when in "bytecode -g" mode, so the difference between Alias and Strict when the bound expression in a Lvar is only the case where the identifier is unused. I'm not 100% sure this doesn't impact the resulting bytecode (and for, instance, jsoo, for which -g is required to get sourcemaps), but I'm much less worried than I was on Friday with this version.

I believe the bug was only morally present for a long time, since the invariant that arguments of Lstaticraise must be pure did not hold previously (because of pattern matching with exception clauses -- but in this case, the Lstaticraise crosses a try...with and is not simplified). AFAICT, the bug was uncovered by the optimization of local functions in 4.08, and was not present in older versions. I've added the reference to this PR and the original issue to that entry (in 4.08).

@xavierleroy
Copy link
Contributor

left a comment

Looks perfect to me! Thanks a lot.

@xavierleroy xavierleroy merged commit e350ebd into ocaml:trunk Apr 1, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

xavierleroy added a commit that referenced this pull request Apr 1, 2019

Use Strict bindings when simplifying Lstaticraise (#8559)
Alias bindings were used before, but as #8558 shows this can be incorrect,
as the argument to `Lstaticraise` may be impure.

Adds a regression test.

Closes: #8558
@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Cherry-picked in 4.08: b7e3ca7

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Concerning "simplif" optimizations in general: I share your suspicion that the bug was already there, but never encountered, before the optimization of functions was introduced. And, yes, ocamlc -g turns let-optimizations off, perhaps too many of them, but that's a topic for further work.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

FTR, 2 things to be careful with:

  • In Closure.close, there is an explicit optimization on Let Alias when a lambda expression is compiled to a ulambda expression which cannot be determined to be pure at that level but result in a constant approximation. Replacing Alias with Strict could in theory disable this optimization.

  • In Matching.compile_orhandlers, there is another optimization which looks like turning Lstaticraise into Let Alias. I did not check, but I assume this is ok in this context.

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.