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

Attribute [@@@zero_alloc check] to turn the check on #1294

Merged
merged 7 commits into from
Apr 12, 2023
Merged

Attribute [@@@zero_alloc check] to turn the check on #1294

merged 7 commits into from
Apr 12, 2023

Conversation

gretay-js
Copy link
Contributor

The new attribtue [@@@zero_alloc check] has the same effect as -zero-alloc-check command line flag. It just turns on the computation of summaries and checking of [@zero_alloc] function annotations on individual files.

The attribute is used for stdlib.ml and other files involved in the special dune compilation rule for stdlib (similarly to [@@@ocaml.flambda_o3]). The rule doesn't take into account ocamlopt_flags specified in stdlib library stanza in ocamll/stdlib/dune file. This due probles is fixed with dune 3.7 but we haven't got it yet.

This attribute is not very useful for user code (except perhaps in small tests), because it won't be able to prove anything about functions with callees in other compilation units not compiled with the check.

To support this new top-level attribute in the usual way, we need access to the flag zero_alloc_check early on from Builtin_attributes which does not know about Flambda_backend_flags. The simplest way to fix it seems to be moving zero_alloc_check to Clflags.

@gretay-js gretay-js mentioned this pull request Apr 10, 2023
Merged
xclerc
xclerc reviewed Apr 10, 2023
View reviewed changes
Copy link
Contributor

@xclerc xclerc left a comment

Choose a reason for hiding this comment

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

Looks good, but it is not clear to me
why the attribute in not present in all
(or at least many more) modules.
The caller/callee issue is an explanation,
but a number of modules (in particular
early in the dependency graph)
provide utility functions with no
dependencies.

@gretay-js
Copy link
Contributor Author

Looks good, but it is not clear to me why the attribute in not present in all (or at least many more) modules. The caller/callee issue is an explanation, but a number of modules (in particular early in the dependency graph) provide utility functions with no dependencies.

The attribute is only needed for files used in the stdlib field of the stdlib library stanza: stdlib.ml std_exit.ml camlinternalAtomic.ml and camlinternalFormatBasics.ml. All other files get -zero-alloc-check flag from ocamlopt_flags of the main context here.

@gretay-js
Copy link
Contributor Author

I have just addded the attribute to camlinternalFormatBasics.ml

I didn't add it initially because this file doesn't have [@@@ ocaml.flambda_o3] so I thought maybe it was special but it doesn't seem to be and it's not only type definitions.

@xclerc
Copy link
Contributor

xclerc commented Apr 10, 2023

Ah, sorry, I did not understand the Dune
part at first. If the solution is meant to be
temporary, we should probably leave a
CR or open an issue to revert it in due
course.

This was referenced Apr 10, 2023
Open
Merged
mshinwell
mshinwell approved these changes Apr 12, 2023
View reviewed changes
Copy link
Collaborator

@mshinwell mshinwell left a comment

Choose a reason for hiding this comment

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

Only files owned by me

@gretay-js
Use [@@@ocaml.zero_alloc check] instead of [@@@zero_alloc check]
c0f6db7 
in stdlib, for consistency with other attributes.
@gretay-js gretay-js merged commit a88d308 into ocaml-flambda:main Apr 12, 2023
mshinwell added a commit to mshinwell/flambda-backend that referenced this pull request Apr 28, 2023
@mshinwell
Squashed 'ocaml/' changes from ea89813..a7d005a
4d0fa29 
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
@ccasin
Squashed 'ocaml/' changes from ea89813..a7d005a
35bb136 
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
@gretay-js gretay-js mentioned this pull request Nov 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xclerc xclerc xclerc approved these changes

@mshinwell mshinwell mshinwell approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@gretay-js @xclerc @mshinwell