zero_alloc attribute payload "assert all" and "ignore"#1296
Merged
gretay-js merged 13 commits intooxcaml:mainfrom Apr 17, 2023
Merged
zero_alloc attribute payload "assert all" and "ignore"#1296gretay-js merged 13 commits intooxcaml:mainfrom
gretay-js merged 13 commits intooxcaml:mainfrom
Conversation
xclerc
reviewed
Apr 10, 2023
mshinwell
approved these changes
Apr 12, 2023
Collaborator
mshinwell
left a comment
There was a problem hiding this comment.
Only files owned by me (under driver/ and Check_attribute)
0877b17 to
95ad55e
Compare
xclerc
reviewed
Apr 14, 2023
xclerc
reviewed
Apr 14, 2023
xclerc
approved these changes
Apr 14, 2023
Contributor
xclerc
left a comment
There was a problem hiding this comment.
I am not sure what users would
expect, so I think we should
document that assert all does
not apply to code that end up
in the entry function. That might
be mildly surprising.
ccasin
approved these changes
Apr 17, 2023
Collaborator
ccasin
left a comment
There was a problem hiding this comment.
A few questions / comments, but this looks good to me.
Regarding the comment in the PR description about the motivation for Ignore_assert_all: It wouldn't be so hard to add an analog of Translattribute.find_attribute that let the caller decide what to do about duplicated attributes instead of warning and throwing them away. But I think even then you need to communicate the information to the backend, because "ignore" isn't the same as "default" if "assert all" is on. So the current approach seems right.
Mostly because using "assert" might be confusing (for human users and parser) with the language construct "assert <e>". This payload was removed from function attribute [@zero_alloc] in a previous commit.
mshinwell
added a commit
to mshinwell/oxcaml
that referenced
this pull request
Apr 28, 2023
a7d005a flambda-backend: Lazy deserialization of cmi files (oxcaml#1322) aa83fa3 flambda-backend: Reinstate previous API for Env.lookup_value (oxcaml#1323) e4007a4 flambda-backend: Lazy substitution into value_declaration (oxcaml#1320) 634b607 flambda-backend: Merge Types.* and Subst.Lazy.* types (oxcaml#1312) cf82708 flambda-backend: Bump magic numbers for 4.14.1-7 (oxcaml#1317) 6470400 flambda-backend: zero_alloc attribute payload "assert all" and "ignore" (oxcaml#1296) bba5248 flambda-backend: Teach `ocamldep` about all the language extensions (oxcaml#1303) 33e97b0 flambda-backend: Change Includemod to work on lazy modtypes (oxcaml#1228) 16e5002 flambda-backend: zero_alloc new warning for unchecked functions (oxcaml#1302) 36b4626 flambda-backend: Attribute [@@@zero_alloc check] to turn the check on (oxcaml#1294) 3b524c6 flambda-backend: Cmm.value_kind cleanup (oxcaml#1091) ec99505 flambda-backend: Fix failure of `check_all_arches` on RISCV (oxcaml#1300) 450bc58 flambda-backend: Backend changes for multiple returns (oxcaml#1268) 84a7a26 flambda-backend: Static check for zero_alloc: ignore allocation that lead to exceptional return (oxcaml#1157) 1723728 flambda-backend: Re-enable parallel build of the runtime (oxcaml#1287) 26ea7f3 flambda-backend: Fix closure marshalling when not in NNP mode (oxcaml#1286) 9b91f2e flambda-backend: Reduce number of caml_apply functions taking/returning "I" and "V" (oxcaml#1272) 1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (oxcaml#1285) cf9be42 flambda-backend: Fix all the no-naked-pointers problems (oxcaml#1282) 8fe089e flambda-backend: Unrevert oxcaml#1131 and fix a Cmm unboxing bug (oxcaml#1284) c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (oxcaml#1283) 2078dce flambda-backend: Add some -dtimings output for the typechecker (oxcaml#1245) 273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (oxcaml#1131) 47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (oxcaml#1274) fd53d38 flambda-backend: Generate *.cms files for merlin (oxcaml#1232) 853f95f flambda-backend: Add tail_mod_const to builtin_attrs (oxcaml#1265) f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (oxcaml#1263) e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (oxcaml#1255) 6f1cd1f flambda-backend: Restore a lost location, needed for merlin (oxcaml#1242) 009332b flambda-backend: Fix merge from ocaml-jst git-subtree-dir: ocaml git-subtree-split: a7d005a
ccasin
added a commit
to ccasin/oxcaml
that referenced
this pull request
Apr 29, 2023
a7d005a flambda-backend: Lazy deserialization of cmi files (oxcaml#1322) aa83fa3 flambda-backend: Reinstate previous API for Env.lookup_value (oxcaml#1323) e4007a4 flambda-backend: Lazy substitution into value_declaration (oxcaml#1320) 634b607 flambda-backend: Merge Types.* and Subst.Lazy.* types (oxcaml#1312) cf82708 flambda-backend: Bump magic numbers for 4.14.1-7 (oxcaml#1317) 6470400 flambda-backend: zero_alloc attribute payload "assert all" and "ignore" (oxcaml#1296) bba5248 flambda-backend: Teach `ocamldep` about all the language extensions (oxcaml#1303) 33e97b0 flambda-backend: Change Includemod to work on lazy modtypes (oxcaml#1228) 16e5002 flambda-backend: zero_alloc new warning for unchecked functions (oxcaml#1302) 36b4626 flambda-backend: Attribute [@@@zero_alloc check] to turn the check on (oxcaml#1294) 3b524c6 flambda-backend: Cmm.value_kind cleanup (oxcaml#1091) ec99505 flambda-backend: Fix failure of `check_all_arches` on RISCV (oxcaml#1300) 450bc58 flambda-backend: Backend changes for multiple returns (oxcaml#1268) 84a7a26 flambda-backend: Static check for zero_alloc: ignore allocation that lead to exceptional return (oxcaml#1157) 1723728 flambda-backend: Re-enable parallel build of the runtime (oxcaml#1287) 26ea7f3 flambda-backend: Fix closure marshalling when not in NNP mode (oxcaml#1286) 9b91f2e flambda-backend: Reduce number of caml_apply functions taking/returning "I" and "V" (oxcaml#1272) 1686928 flambda-backend: Restore Cmm unboxing behaviour inside regions (oxcaml#1285) cf9be42 flambda-backend: Fix all the no-naked-pointers problems (oxcaml#1282) 8fe089e flambda-backend: Unrevert oxcaml#1131 and fix a Cmm unboxing bug (oxcaml#1284) c4143c3 flambda-backend: Revert "Make Selectgen treat region boundaries more precisely" (oxcaml#1283) 2078dce flambda-backend: Add some -dtimings output for the typechecker (oxcaml#1245) 273a7f9 flambda-backend: Make Selectgen treat region boundaries more precisely (oxcaml#1131) 47610e6 flambda-backend: Bump magic numbers for 4.14.1-6 (oxcaml#1274) fd53d38 flambda-backend: Generate *.cms files for merlin (oxcaml#1232) 853f95f flambda-backend: Add tail_mod_const to builtin_attrs (oxcaml#1265) f9ef051 flambda-backend: Fix issue caused by effects of gadt expansion in mode cross check (oxcaml#1263) e9ffcf8 flambda-backend: Fix dependencies for regenerating Flambda2 parser, tests (oxcaml#1255) 6f1cd1f flambda-backend: Restore a lost location, needed for merlin (oxcaml#1242) 009332b flambda-backend: Fix merge from ocaml-jst git-subtree-dir: ocaml git-subtree-split: a7d005a
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
on top of #1294, only the last 3 commits are new.
This is somewhat experimental. The attributes make it easier to annotate entire files that are mostly not allocating.
[@@@zero_alloc assert all]at top-level of a compilation unit is the same as annotating all functions in the file with[@zero_alloc]. Note that it performs a relaxed check, and there is no corresponding attribute for strict check yet.[@zero_alloc ignore]on a function cancels the effect of[@@@zero_alloc all]for that function. That is, compute the summary of this function, but don't check/assert that the function is not allocating. Note that[@zero_alloc ignore]is not the same as[@zero_alloc assume]which does not check the function but sets it's summary is as non-allocating ). If the file is not annotated with[@@@zero_alloc assert all]then[@zero_alloc ignore]on a function in that file does not do anything.Ideally, these annotations should be resolved in
TranslattributeandBuiltin_attrintoLambda.check_attribute.Default_checkorLambda.check_attribute.Check _but I couldn't figure how to do it forignorepayload. The problem is when an attribute is parsed inTranslattribute, it is not clear whether it is the onlyzero_allocattribute on the function or not. The proposed implementation just adds another constructor for itLambda.check_attribute.Ignore_assert_alland resolves it in the backend. The downside is that it needs to be propagated all the way down. An alternative would be to resolve it during the translation fromlambdain the middle-end, which would result in some code duplication and changes in the middle-end in places that should not care about this check.