Fix computation of extra_args during CPS conversion of Lstaticraise #126
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mshinwell
added
bug
|
One other detail: this problem was only noted because |
|
This only affects the Flambda 2 part of the tree and will be reviewed later. |
mshinwell
added a commit
to mshinwell/oxcaml
that referenced
this pull request
Aug 4, 2021
ccasin
added a commit
to ccasin/flambda-backend
that referenced
this pull request
Mar 12, 2023
5e7dfce331 Merge flambda-backend changes 9f7f2d24a7 Edit script for test merge 1ae71ce59d Review comments (oxcaml#6) 1845365bd0 Layouts version 1 aba6294 Immediacy rework (oxcaml#122) cf4eeef Add no-stack-allocation variant of some tests that print lambda (oxcaml#133) 8f22438 Fully switch over Jane Street Merlin support to `.local-*` (oxcaml#136) 5482a8d Remove `Lev_module_definition` from lambda (oxcaml#135) 261e016 Change modular extensions to produce `AST_desc` types (oxcaml#132) 0760c82 Disable module patterns in comprehensions (oxcaml#131) 6acac80 Add Ctype global state debug printers (oxcaml#130) bc32037 Enable support for Jane Street's internal Merlin configuration (oxcaml#64) d1a8d03 Split out `Clflags.Extension` into a new file, `Language_extension` (oxcaml#125) 435de6d Fix bootstrap and add legacy CI (oxcaml#126) 7e5a626 Improve the API of language extensions to better support upstream compatibility (and also tooling) (oxcaml#13) c4e17b0 Replace var with local for faster mode checking (oxcaml#53) 6d477d8 Merge pull request oxcaml#124 from riaqn/merge-backend d737533 minor fixes after merge f1710d6 Merge flambda-backend changes cc18534 Just run make boostrap (oxcaml#123) git-subtree-dir: ocaml git-subtree-split: 5e7dfce331d0e39c695fab9b00e3d2cda7d9ebb4
ccasin
added a commit
that referenced
this pull request
Mar 24, 2023
ea89813 Merge pull request #154 from ocaml-flambda/merge-flambda-backend 23cf5a5 Merge flambda-backend changes b3af0c4 Unboxed types version 3 tests (#82) 1282d16 Functions with no clauses aren't local-returning (#149) 15d38c0 `make install` puts ocamlc at workspace root (#152) b4928ee Remove -absname to improve build errors (#151) f5b5e49 Remove `type_unpacks` (#150) 50d54db Remove arity-interrupting elaboration of module patterns (#146) 4382869 Remove the need to manually update the `tools/debug_printers` file (#148) 06a1d91 Add `promote-failed` targets to the Makefiles (#144) d04eb58 Cleanup of comprehensions and immutable arrays (#127) a45df79 Add a Module_strengthening extension (#142) 163c4b9 Add support for extensions in module types (#141) 74aa974 Some small patch-ups around matching on extensions (#140) 07127fe Remove raw_body from modular extensions setup (#137) 3f9bd64 Don't copy when resolving aliases in try_modtypes (#143) aba6294 Immediacy rework (#122) cf4eeef Add no-stack-allocation variant of some tests that print lambda (#133) 8f22438 Fully switch over Jane Street Merlin support to `.local-*` (#136) 5482a8d Remove `Lev_module_definition` from lambda (#135) 261e016 Change modular extensions to produce `AST_desc` types (#132) 0760c82 Disable module patterns in comprehensions (#131) 6acac80 Add Ctype global state debug printers (#130) bc32037 Enable support for Jane Street's internal Merlin configuration (#64) d1a8d03 Split out `Clflags.Extension` into a new file, `Language_extension` (#125) 435de6d Fix bootstrap and add legacy CI (#126) 7e5a626 Improve the API of language extensions to better support upstream compatibility (and also tooling) (#13) c4e17b0 Replace var with local for faster mode checking (#53) 6d477d8 Merge pull request #124 from riaqn/merge-backend d737533 minor fixes after merge f1710d6 Merge flambda-backend changes cc18534 Just run make boostrap (#123) git-subtree-dir: ocaml git-subtree-split: ea89813
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a subtle bug in CPS conversion, which took many hours to find, that can be triggered by the example in the diff. Prior to this PR, the raw Flambda terms were wrong: they would return
initinstead of!mfrom the functionf.The example produces an unusual Lambda code sequence (in the comment in the diff) which involves having a
for-loop as the argument toLstaticraise(printed as "exit") in conjunction with a mutable variable that is required at the correspondingLstaticcatchhandler. It is necessary to get the correct "instance" of the mutable variable, converted during CPS conversion to multiple immutable ones, at the exits of thefor-loop. This means that the environment in the meta-level continuation passed tocps_non_tail_list(originally on line 901/902) must be used for computingextra_args, not the environment e.g. on line 896 as was happening before. Using the environment on line 896 causes theLstaticcatchhandler to receive the snapshot of the mutable variable as it was before thefor-loop, not after the requisite number of iterations.