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.

Sign up for GitHub

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

Jump to bottom

More assertions for local alloc and letrec #1081

Merged
merged 6 commits into from
Feb 21, 2023
Merged

More assertions for local alloc and letrec #1081

merged 6 commits into from
Feb 21, 2023

Conversation

stedolan
Copy link
Contributor

The bindings of a let rec block cannot be locally allocated unless they're all functions. (Locals do not support stack-allocating mutually recursive blocks, so let rec can't be local unless it's a single closure block)

This is enforced by the typechecker. This patch adds some assertions to the backend, so that we get a failure rather than a miscompilation if the typechecker doesn't do its job. (Similar assertions are currently present in Translcore, but it's sort of the wrong place for them since it's neither the point where either the decision is made nor where a miscompilation would occur)

mshinwell
mshinwell previously requested changes Jan 18, 2023
View reviewed changes
middle_end/flambda2/from_lambda/dissect_letrec.ml Outdated Show resolved Hide resolved
@mshinwell
Copy link
Collaborator

@chambart could you please review the Dissect_letrec part of this?

@riaqn riaqn mentioned this pull request Jan 19, 2023
Merged
@stedolan
Copy link
Contributor Author

The previous implementation had an over-eager assertion in Cmmgen, so there was a test failure in non-Flambda2 builds. Fixed now.

lthls
lthls reviewed Jan 23, 2023
View reviewed changes
backend/cmmgen.ml Outdated Show resolved Hide resolved
@stedolan
Copy link
Contributor Author

@lthls spotted an error in the assertions in Cmmgen.expr_size, just pushed a fix.

@stedolan
Copy link
Contributor Author

stedolan commented Feb 8, 2023

This is rebased and I think it's ready to merge. @lthls happy with the current check?

lthls
lthls approved these changes Feb 8, 2023
View reviewed changes
Copy link
Contributor

@lthls lthls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handling of local allocations and regions in Dissect_letrec looks wrong but this PR likely doesn't make it worse. I think @chambart will have some more precise comments. The rest looks fine.

backend/cmmgen.ml
RHS_block (mode, List.length args)
| Uprim(Pmakearray(Pfloatarray, _, mode), args, _) ->
RHS_floatblock (mode, List.length args)
| Uprim(Pmakearray(Pgenarray, _, _mode), _, _) ->
(* Pgenarray is excluded from recursive bindings by the
check in Translcore.check_recursive_lambda *)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is quite out of date. I suspect we can remove all the Pmakearray cases, as they're normally always followed by Pduparray, but I guess this can be handled in a separate PR.

@chambart
Copy link
Contributor

chambart commented Feb 8, 2023

I looked a bit at the local stuffs in dissect letrec and to be honnest, I'm not convinced that the need_region stuff works at all the way we might want to. I would be happier removing that and considering the Region case as a non recursive one.

My logic for that being that: if some part of the recursive value is local, then everything has to be local. So if some part of the letrec are below a new region then the local values allocated in that can't be part of the recursive value: those are here only for side effects, hence it's ok to keep that part as is.

@stedolan
Copy link
Contributor Author

@chambart

I looked a bit at the local stuffs in dissect letrec and to be honnest, I'm not convinced that the need_region stuff works at all the way we might want to. I would be happier removing that and considering the Region case as a non recursive one.

Sure, but that sounds like a separate PR? Unless you think the assertion added here is invalid because of this issue?

@mshinwell
Copy link
Collaborator

It sounds like we can do a separate PR for the Dissect_letrec changes. @lthls @chambart I've added this to the project board.

@mshinwell mshinwell merged commit b0e1527 into ocaml-flambda:main Feb 21, 2023
ccasin added a commit to ccasin/flambda-backend that referenced this pull request Mar 12, 2023
@ccasin
Squashed 'ocaml/' changes from c703f5f77..77434f02e
d93152b 
77434f02e flambda-backend: Bump magic numbers for 4.14.1-5 (ocaml-flambda#1190)
3a78e83c9 flambda-backend: Revert "Instance compilation units" (ocaml-flambda#1175)
9a683c928 flambda-backend: Move compute_layout to Lambda (ocaml-flambda#1167)
0ea58e9a3 flambda-backend: Revert "Add a proper top and bottom layout" (ocaml-flambda#1169)
1e5e23a1b flambda-backend: Add hint for common misplaced [@unboxed] attribute (ocaml-flambda#1164)
10f870ac9 flambda-backend: Add a proper top and bottom layout (ocaml-flambda#1158)
d1be56335 flambda-backend: Don't warn about misplaced attributes in -i mode (ocaml-flambda#1163)
feefcaac8 flambda-backend: Remove immutable arrays from stdlib.ml and stdlib.mli (ocaml-flambda#1154)
ba101be42 flambda-backend: Shrink the ref_table if it grows large (ocaml-flambda#1156)
777fda7b9 flambda-backend: Preserve backtraces from failing `Lazy_backtrack` computations (ocaml-flambda#805)
56d014e05 flambda-backend: Remove remaining `layout_top` after ocaml-flambda#1084 (ocaml-flambda#1138)
dc1c1ce89 flambda-backend: Layouts for parameters in lambda & remove most layout_top (ocaml-flambda#1084)
49fea7805 flambda-backend: Instance compilation units (ocaml-flambda#1113)
1127fd2f8 flambda-backend: Merge pull request ocaml-flambda#1143 from riaqn/merge-ocaml-jst
f458733c8 flambda-backend: fix things after merge
b43d38509 flambda-backend: File magic updates (ocaml-flambda#306)
9f604aaea flambda-backend: Merge ocaml-jst
1f5461386 flambda-backend: More assertions for local alloc and letrec (ocaml-flambda#1081)
0c95280ab flambda-backend: Remove most layout_top in closure (ocaml-flambda#1127)
8ad48b5b3 flambda-backend: Use only `.ocamlformat*` to enable or disable ocamlformat (ocaml-flambda#1135)
1aa2885a0 flambda-backend: Build all boot targets in `make hacking` (ocaml-flambda#1133)
6b6c25a85 flambda-backend: Hide `Compilation_unit.t`'s definition in a submodule (ocaml-flambda#1134)
529d66b6f flambda-backend: Compile refactor (ocaml-flambda#1096)
df798c1c9 flambda-backend: Propagates layouts through Flambda1 (ocaml-flambda#1115)
9bce50be5 flambda-backend: Add `gc-timings` to collect timing information from the GC (ocaml-flambda#1089)
d431d3b83 flambda-backend: Use the type of primitive declarations to make their layout (ocaml-flambda#1118)
8da887e0f flambda-backend: Changes to arity in clambda (ocaml-flambda#1106)
db20e9724 flambda-backend: Fix simplify-exits (ocaml-flambda#1108)
6a63906ee flambda-backend: Add layout on Lregion (ocaml-flambda#1107)
c562fb385 flambda-backend: caml_{curry,apply,send}* for unboxed types (ocaml-flambda#1104)
50ee311bc flambda-backend: Temporary fix for incorrect layout for result of custom and operators (ocaml-flambda#1119)
c89512d64 flambda-backend: More error checking for natdynlink symbols (ocaml-flambda#1005)
27d68bf7e flambda-backend: Flambda1 region deletion and locals fixes (ocaml-flambda#1000)
0e3e0576e flambda-backend: Add identifiers for instantiated functor units (ocaml-flambda#1092)
4b75b460d flambda-backend: Fix build system under ocaml/runtime (ocaml-flambda#1085)
7c9fc3215 flambda-backend: Fix valid character range computation for compilation units (ocaml-flambda#1110)
40c754a7d flambda-backend: Add result layout in Lapply and Lsend (ocaml-flambda#1102)
cedaea1e0 flambda-backend: Add layout type in Lambda (ocaml-flambda#1032)
47c0e23a2 flambda-backend: Configure: Add flag to use legacy library layout (ocaml-flambda#1098)
eed588881 flambda-backend: Restructure LIBDIR: Move Dynlink, Str and Unix to sub-directories (port part of ocaml/PR11198) (ocaml-flambda#1094)
5b98caf9d flambda-backend: Revert "Restructure LIBDIR" (ocaml-flambda#1093)
46f1b16d7 flambda-backend: Remove a stray character from dynlink Makefile (ocaml-flambda#1090)
e5154424a flambda-backend: Restructure LIBDIR: Move Dynlink, Str and Unix to sub-directories (port PR#11198) (ocaml-flambda#895)
9307e7c50 flambda-backend: inline fast path of caml_applyN (ocaml-flambda#934)
3244386e6 flambda-backend: Bump magic numbers (ocaml-flambda#1077)
7c9e15d2c flambda-backend: Fix bug in arguments to `get_unit_info` (ocaml-flambda#1069)
a2f4c9e07 flambda-backend: Closure rename static catch (ocaml-flambda#1070)
2ee705a5b flambda-backend: Backport ocaml-flambda#946 to ocaml/ subfolder (ocaml-flambda#1061)
569703fb6 flambda-backend: Merge ocaml-jst#97 (ocaml-flambda#1063)
7b1779a77 flambda-backend: Port value_kind changes to testsuite/tools to fix parsecmm.mly (ocaml-flambda#1060)
49505d183 flambda-backend: Bugfix for Ctype.nondep_type (ocaml-flambda#1059)
654c63cf7 flambda-backend: Backport ocaml-flambda#295 kind changes to ocaml/ subfolder (ocaml-flambda#1018)
50a9ce0bf flambda-backend: Fix extern_closure_up_to_env (ocaml-flambda#1053)
bc2c78d2e flambda-backend: Remove obsolete pack-related code (ocaml-flambda#1051)
872ff3818 flambda-backend: Fix error-prone syntax resulting from reformatting (ocaml-flambda#1050)
d03b346cf flambda-backend: Fix different .cmi files being produced by ocamlopt and ocamlc when using -pack (ocaml-flambda#1049)
90ee37b48 flambda-backend: Revert "Revert "Use Import_info.t in Cmt_format"" (ocaml-flambda#1045)
ac12d9072 flambda-backend: Fix version number (ocaml-flambda#1043)
75e01541e flambda-backend: Revert "Use Import_info.t in Cmt_format" (ocaml-flambda#1042)
ecab74c38 flambda-backend: Use Import_info.t in Cmt_format (ocaml-flambda#1037)
7661d4d47 flambda-backend: Bootstrap
1098a56c6 flambda-backend: Update .depend files
a7292dae2 flambda-backend: Revert ocaml/toplevel/native/dune
758447903 flambda-backend: Remove alloc-check from ocaml/

git-subtree-dir: ocaml
git-subtree-split: 77434f02edabf1a8b17372747bf301a74879df0f
ccasin added a commit that referenced this pull request Mar 24, 2023
@ccasin
Squashed 'ocaml/' changes from c703f5f77..bcae5ff33
2376a0b 
bcae5ff33 flambda-backend: Bump buffer size to avoid gcc warning (#1247)
d6c170bac flambda-backend: Remove `-absname` to improve dune build errors (#1233)
1b2bcb487 flambda-backend: Put frametables in .text with relative offsets for return addresses. (#1227)
999d523b4 flambda-backend: Small optimisation for caml_modify (#1226)
b16493b31 flambda-backend: Add timestamps to GC logs (#1229)
95f7e809e flambda-backend: Provide a no-naked-pointers runtime and use it for the compiler (#1224)
ba775819d flambda-backend: Replace assertion with a match statement (#1225)
cfb3cd242 flambda-backend: Memoise last substitution composition (#1209)
1c4db4405 flambda-backend: Port PR1202 and PR1205 to the ocaml/ subtree (#1211)
577410ee7 flambda-backend: Correctly stack debuginfo for inlined body in classic mode (#1152)
6733de6a5 flambda-backend: Fix dune install in otherlibs: missing cmt and typo. (#1194)
4c97d260e flambda-backend: Unboxed numbers (#1165)
1ad7252a0 flambda-backend: Revert "Revert "Add a proper top and bottom layout" (#1169)" (#1191)
dea4b3e6b flambda-backend: Don't get the layout of arguments from patterns (#1179)
6d3e85b23 flambda-backend: Don't copy when resolving aliases in try_modtypes (#1184)
73e52b748 flambda-backend: Clarify the types used for static jump/catch (#1180)
273a40df2 flambda-backend: Reenable backtrace testsuite folder for flambda2 (#1161)
77434f02e flambda-backend: Bump magic numbers for 4.14.1-5 (#1190)
3a78e83c9 flambda-backend: Revert "Instance compilation units" (#1175)
9a683c928 flambda-backend: Move compute_layout to Lambda (#1167)
0ea58e9a3 flambda-backend: Revert "Add a proper top and bottom layout" (#1169)
1e5e23a1b flambda-backend: Add hint for common misplaced [@unboxed] attribute (#1164)
10f870ac9 flambda-backend: Add a proper top and bottom layout (#1158)
d1be56335 flambda-backend: Don't warn about misplaced attributes in -i mode (#1163)
feefcaac8 flambda-backend: Remove immutable arrays from stdlib.ml and stdlib.mli (#1154)
ba101be42 flambda-backend: Shrink the ref_table if it grows large (#1156)
777fda7b9 flambda-backend: Preserve backtraces from failing `Lazy_backtrack` computations (#805)
56d014e05 flambda-backend: Remove remaining `layout_top` after #1084 (#1138)
dc1c1ce89 flambda-backend: Layouts for parameters in lambda & remove most layout_top (#1084)
49fea7805 flambda-backend: Instance compilation units (#1113)
1127fd2f8 flambda-backend: Merge pull request #1143 from riaqn/merge-ocaml-jst
f458733c8 flambda-backend: fix things after merge
b43d38509 flambda-backend: File magic updates (#306)
9f604aaea flambda-backend: Merge ocaml-jst
1f5461386 flambda-backend: More assertions for local alloc and letrec (#1081)
0c95280ab flambda-backend: Remove most layout_top in closure (#1127)
8ad48b5b3 flambda-backend: Use only `.ocamlformat*` to enable or disable ocamlformat (#1135)
1aa2885a0 flambda-backend: Build all boot targets in `make hacking` (#1133)
6b6c25a85 flambda-backend: Hide `Compilation_unit.t`'s definition in a submodule (#1134)
529d66b6f flambda-backend: Compile refactor (#1096)
df798c1c9 flambda-backend: Propagates layouts through Flambda1 (#1115)
9bce50be5 flambda-backend: Add `gc-timings` to collect timing information from the GC (#1089)
d431d3b83 flambda-backend: Use the type of primitive declarations to make their layout (#1118)
8da887e0f flambda-backend: Changes to arity in clambda (#1106)
db20e9724 flambda-backend: Fix simplify-exits (#1108)
6a63906ee flambda-backend: Add layout on Lregion (#1107)
c562fb385 flambda-backend: caml_{curry,apply,send}* for unboxed types (#1104)
50ee311bc flambda-backend: Temporary fix for incorrect layout for result of custom and operators (#1119)
c89512d64 flambda-backend: More error checking for natdynlink symbols (#1005)
27d68bf7e flambda-backend: Flambda1 region deletion and locals fixes (#1000)
0e3e0576e flambda-backend: Add identifiers for instantiated functor units (#1092)
4b75b460d flambda-backend: Fix build system under ocaml/runtime (#1085)
7c9fc3215 flambda-backend: Fix valid character range computation for compilation units (#1110)
40c754a7d flambda-backend: Add result layout in Lapply and Lsend (#1102)
cedaea1e0 flambda-backend: Add layout type in Lambda (#1032)
47c0e23a2 flambda-backend: Configure: Add flag to use legacy library layout (#1098)
eed588881 flambda-backend: Restructure LIBDIR: Move Dynlink, Str and Unix to sub-directories (port part of ocaml/PR11198) (#1094)
5b98caf9d flambda-backend: Revert "Restructure LIBDIR" (#1093)
46f1b16d7 flambda-backend: Remove a stray character from dynlink Makefile (#1090)
e5154424a flambda-backend: Restructure LIBDIR: Move Dynlink, Str and Unix to sub-directories (port PR#11198) (#895)
9307e7c50 flambda-backend: inline fast path of caml_applyN (#934)
3244386e6 flambda-backend: Bump magic numbers (#1077)
7c9e15d2c flambda-backend: Fix bug in arguments to `get_unit_info` (#1069)
a2f4c9e07 flambda-backend: Closure rename static catch (#1070)
2ee705a5b flambda-backend: Backport #946 to ocaml/ subfolder (#1061)
569703fb6 flambda-backend: Merge ocaml-jst#97 (#1063)
7b1779a77 flambda-backend: Port value_kind changes to testsuite/tools to fix parsecmm.mly (#1060)
49505d183 flambda-backend: Bugfix for Ctype.nondep_type (#1059)
654c63cf7 flambda-backend: Backport #295 kind changes to ocaml/ subfolder (#1018)
50a9ce0bf flambda-backend: Fix extern_closure_up_to_env (#1053)
bc2c78d2e flambda-backend: Remove obsolete pack-related code (#1051)
872ff3818 flambda-backend: Fix error-prone syntax resulting from reformatting (#1050)
d03b346cf flambda-backend: Fix different .cmi files being produced by ocamlopt and ocamlc when using -pack (#1049)
90ee37b48 flambda-backend: Revert "Revert "Use Import_info.t in Cmt_format"" (#1045)
ac12d9072 flambda-backend: Fix version number (#1043)
75e01541e flambda-backend: Revert "Use Import_info.t in Cmt_format" (#1042)
ecab74c38 flambda-backend: Use Import_info.t in Cmt_format (#1037)
7661d4d47 flambda-backend: Bootstrap
1098a56c6 flambda-backend: Update .depend files
a7292dae2 flambda-backend: Revert ocaml/toplevel/native/dune
758447903 flambda-backend: Remove alloc-check from ocaml/

git-subtree-dir: ocaml
git-subtree-split: bcae5ff335d548b92752f53817d5b08ab2b98ff9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mshinwell mshinwell mshinwell left review comments

@lthls lthls lthls approved these changes

@gretay-js gretay-js Awaiting requested review from gretay-js

@xclerc xclerc Awaiting requested review from xclerc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stedolan @mshinwell @chambart @lthls