-
Notifications
You must be signed in to change notification settings - Fork 77
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
Unrevert #1131 and fix a Cmm unboxing bug #1284
Merged
Merged
Conversation
This file contains 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
ocaml-flambda#1283)" This reverts commit abd91a9.
Local regions should not count as "tail" for Cmm unboxing purposes.
lpw25
approved these changes
Apr 3, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks good, although it could perhaps be more relaxed in some cases.
ccasin
added a commit
to ccasin/flambda-backend
that referenced
this pull request
Apr 4, 2023
1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (ocaml-flambda#1285) cf9be42 flambda-backend: Fix all the no-naked-pointers problems (ocaml-flambda#1282) 8fe089e flambda-backend: Unrevert ocaml-flambda#1131 and fix a Cmm unboxing bug (ocaml-flambda#1284) c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (ocaml-flambda#1283) 2078dce flambda-backend: Add some -dtimings output for the typechecker (ocaml-flambda#1245) 273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (ocaml-flambda#1131) 47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (ocaml-flambda#1274) fd53d38 flambda-backend: Generate *.cms files for merlin (ocaml-flambda#1232) 853f95f flambda-backend: Add tail_mod_const to builtin_attrs (ocaml-flambda#1265) f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (ocaml-flambda#1263) e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (ocaml-flambda#1255) 6f1cd1f flambda-backend: Restore a lost location, needed for merlin (ocaml-flambda#1242) 009332b flambda-backend: Fix merge from ocaml-jst git-subtree-dir: ocaml git-subtree-split: 1686928
ccasin
added a commit
to ccasin/flambda-backend
that referenced
this pull request
Apr 17, 2023
1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (ocaml-flambda#1285) cf9be42 flambda-backend: Fix all the no-naked-pointers problems (ocaml-flambda#1282) 8fe089e flambda-backend: Unrevert ocaml-flambda#1131 and fix a Cmm unboxing bug (ocaml-flambda#1284) c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (ocaml-flambda#1283) 2078dce flambda-backend: Add some -dtimings output for the typechecker (ocaml-flambda#1245) 273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (ocaml-flambda#1131) 47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (ocaml-flambda#1274) fd53d38 flambda-backend: Generate *.cms files for merlin (ocaml-flambda#1232) 853f95f flambda-backend: Add tail_mod_const to builtin_attrs (ocaml-flambda#1265) f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (ocaml-flambda#1263) e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (ocaml-flambda#1255) 6f1cd1f flambda-backend: Restore a lost location, needed for merlin (ocaml-flambda#1242) 009332b flambda-backend: Fix merge from ocaml-jst git-subtree-dir: ocaml git-subtree-split: 1686928
ccasin
added a commit
to ccasin/flambda-backend
that referenced
this pull request
Apr 26, 2023
1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (ocaml-flambda#1285) cf9be42 flambda-backend: Fix all the no-naked-pointers problems (ocaml-flambda#1282) 8fe089e flambda-backend: Unrevert ocaml-flambda#1131 and fix a Cmm unboxing bug (ocaml-flambda#1284) c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (ocaml-flambda#1283) 2078dce flambda-backend: Add some -dtimings output for the typechecker (ocaml-flambda#1245) 273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (ocaml-flambda#1131) 47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (ocaml-flambda#1274) fd53d38 flambda-backend: Generate *.cms files for merlin (ocaml-flambda#1232) 853f95f flambda-backend: Add tail_mod_const to builtin_attrs (ocaml-flambda#1265) f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (ocaml-flambda#1263) e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (ocaml-flambda#1255) 6f1cd1f flambda-backend: Restore a lost location, needed for merlin (ocaml-flambda#1242) 009332b flambda-backend: Fix merge from ocaml-jst git-subtree-dir: ocaml git-subtree-split: 1686928
ccasin
added a commit
to ccasin/flambda-backend
that referenced
this pull request
Apr 27, 2023
1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (ocaml-flambda#1285) cf9be42 flambda-backend: Fix all the no-naked-pointers problems (ocaml-flambda#1282) 8fe089e flambda-backend: Unrevert ocaml-flambda#1131 and fix a Cmm unboxing bug (ocaml-flambda#1284) c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (ocaml-flambda#1283) 2078dce flambda-backend: Add some -dtimings output for the typechecker (ocaml-flambda#1245) 273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (ocaml-flambda#1131) 47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (ocaml-flambda#1274) fd53d38 flambda-backend: Generate *.cms files for merlin (ocaml-flambda#1232) 853f95f flambda-backend: Add tail_mod_const to builtin_attrs (ocaml-flambda#1265) f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (ocaml-flambda#1263) e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (ocaml-flambda#1255) 6f1cd1f flambda-backend: Restore a lost location, needed for merlin (ocaml-flambda#1242) 009332b flambda-backend: Fix merge from ocaml-jst git-subtree-dir: ocaml git-subtree-split: 1686928
mshinwell
added a commit
to mshinwell/flambda-backend
that referenced
this pull request
Apr 28, 2023
a7d005a flambda-backend: Lazy deserialization of cmi files (ocaml-flambda#1322) aa83fa3 flambda-backend: Reinstate previous API for Env.lookup_value (ocaml-flambda#1323) e4007a4 flambda-backend: Lazy substitution into value_declaration (ocaml-flambda#1320) 634b607 flambda-backend: Merge Types.* and Subst.Lazy.* types (ocaml-flambda#1312) cf82708 flambda-backend: Bump magic numbers for 4.14.1-7 (ocaml-flambda#1317) 6470400 flambda-backend: zero_alloc attribute payload "assert all" and "ignore" (ocaml-flambda#1296) bba5248 flambda-backend: Teach `ocamldep` about all the language extensions (ocaml-flambda#1303) 33e97b0 flambda-backend: Change Includemod to work on lazy modtypes (ocaml-flambda#1228) 16e5002 flambda-backend: zero_alloc new warning for unchecked functions (ocaml-flambda#1302) 36b4626 flambda-backend: Attribute [@@@zero_alloc check] to turn the check on (ocaml-flambda#1294) 3b524c6 flambda-backend: Cmm.value_kind cleanup (ocaml-flambda#1091) ec99505 flambda-backend: Fix failure of `check_all_arches` on RISCV (ocaml-flambda#1300) 450bc58 flambda-backend: Backend changes for multiple returns (ocaml-flambda#1268) 84a7a26 flambda-backend: Static check for zero_alloc: ignore allocation that lead to exceptional return (ocaml-flambda#1157) 1723728 flambda-backend: Re-enable parallel build of the runtime (ocaml-flambda#1287) 26ea7f3 flambda-backend: Fix closure marshalling when not in NNP mode (ocaml-flambda#1286) 9b91f2e flambda-backend: Reduce number of caml_apply functions taking/returning "I" and "V" (ocaml-flambda#1272) 1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (ocaml-flambda#1285) cf9be42 flambda-backend: Fix all the no-naked-pointers problems (ocaml-flambda#1282) 8fe089e flambda-backend: Unrevert ocaml-flambda#1131 and fix a Cmm unboxing bug (ocaml-flambda#1284) c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (ocaml-flambda#1283) 2078dce flambda-backend: Add some -dtimings output for the typechecker (ocaml-flambda#1245) 273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (ocaml-flambda#1131) 47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (ocaml-flambda#1274) fd53d38 flambda-backend: Generate *.cms files for merlin (ocaml-flambda#1232) 853f95f flambda-backend: Add tail_mod_const to builtin_attrs (ocaml-flambda#1265) f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (ocaml-flambda#1263) e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (ocaml-flambda#1255) 6f1cd1f flambda-backend: Restore a lost location, needed for merlin (ocaml-flambda#1242) 009332b flambda-backend: Fix merge from ocaml-jst git-subtree-dir: ocaml git-subtree-split: a7d005a
ccasin
added a commit
to ccasin/flambda-backend
that referenced
this pull request
Apr 29, 2023
a7d005a flambda-backend: Lazy deserialization of cmi files (ocaml-flambda#1322) aa83fa3 flambda-backend: Reinstate previous API for Env.lookup_value (ocaml-flambda#1323) e4007a4 flambda-backend: Lazy substitution into value_declaration (ocaml-flambda#1320) 634b607 flambda-backend: Merge Types.* and Subst.Lazy.* types (ocaml-flambda#1312) cf82708 flambda-backend: Bump magic numbers for 4.14.1-7 (ocaml-flambda#1317) 6470400 flambda-backend: zero_alloc attribute payload "assert all" and "ignore" (ocaml-flambda#1296) bba5248 flambda-backend: Teach `ocamldep` about all the language extensions (ocaml-flambda#1303) 33e97b0 flambda-backend: Change Includemod to work on lazy modtypes (ocaml-flambda#1228) 16e5002 flambda-backend: zero_alloc new warning for unchecked functions (ocaml-flambda#1302) 36b4626 flambda-backend: Attribute [@@@zero_alloc check] to turn the check on (ocaml-flambda#1294) 3b524c6 flambda-backend: Cmm.value_kind cleanup (ocaml-flambda#1091) ec99505 flambda-backend: Fix failure of `check_all_arches` on RISCV (ocaml-flambda#1300) 450bc58 flambda-backend: Backend changes for multiple returns (ocaml-flambda#1268) 84a7a26 flambda-backend: Static check for zero_alloc: ignore allocation that lead to exceptional return (ocaml-flambda#1157) 1723728 flambda-backend: Re-enable parallel build of the runtime (ocaml-flambda#1287) 26ea7f3 flambda-backend: Fix closure marshalling when not in NNP mode (ocaml-flambda#1286) 9b91f2e flambda-backend: Reduce number of caml_apply functions taking/returning "I" and "V" (ocaml-flambda#1272) 1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (ocaml-flambda#1285) cf9be42 flambda-backend: Fix all the no-naked-pointers problems (ocaml-flambda#1282) 8fe089e flambda-backend: Unrevert ocaml-flambda#1131 and fix a Cmm unboxing bug (ocaml-flambda#1284) c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (ocaml-flambda#1283) 2078dce flambda-backend: Add some -dtimings output for the typechecker (ocaml-flambda#1245) 273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (ocaml-flambda#1131) 47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (ocaml-flambda#1274) fd53d38 flambda-backend: Generate *.cms files for merlin (ocaml-flambda#1232) 853f95f flambda-backend: Add tail_mod_const to builtin_attrs (ocaml-flambda#1265) f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (ocaml-flambda#1263) e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (ocaml-flambda#1255) 6f1cd1f flambda-backend: Restore a lost location, needed for merlin (ocaml-flambda#1242) 009332b flambda-backend: Fix merge from ocaml-jst git-subtree-dir: ocaml git-subtree-split: a7d005a
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.
Cmm unboxing has been generating invalid Cmm code for quite a while, by pushing unboxing operations even into
Cregion ...
. If theCregion
ends in a tail call, this pushes the tail call out of tail position, making an invalidCapply
(saying that it closes a region while not actually being at the end of a region).This caused build failures after #1131 was merged. (Before that, Selectgen didn't pay much attention to the region-closing flags in these cases, but after #1131 it is more careful).
The fix here is to stop unboxing in these cases, by removing them from the Cmm
(iter/map)_tail
functions. I've checked the uses of these functions: apart from Cmm unboxing (the fix here), all of the other uses already special-case Cregion and Ctail, and so are unaffected by this change.