Skip to content

Do not delete code of functions marked for zero-alloc check#1378

Merged
mshinwell merged 1 commit intooxcaml:mainfrom
mshinwell:flambda2-zero-alloc-dont-delete
May 25, 2023
Merged

Do not delete code of functions marked for zero-alloc check#1378
mshinwell merged 1 commit intooxcaml:mainfrom
mshinwell:flambda2-zero-alloc-dont-delete

Conversation

@mshinwell
Copy link
Copy Markdown
Collaborator

@mshinwell mshinwell commented May 11, 2023

This should fix #1273. It currently only works for Flambda 2. @gretay-js could you please test to see if this suffices?

Here is an interesting example to try:

let[@zero_alloc] bar x =
  let[@zero_alloc] [@inline] g2 x y = x, Some y in
  fst (g2 x (x + 1))

What should happen for the annotation to check the zero-alloc property for all functions in a file? Do all functions in Lambda end up annotated with attributes as normal, or does a different flag need to be tested?

@mshinwell mshinwell added the flambda2 Prerequisite for, or part of, flambda2 label May 11, 2023
Gbury
Gbury approved these changes May 11, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Gbury Gbury left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but I'm not very knowledgeable about simplify_set_of_closures.ml so another review for that file might be useful.

@gretay-js
Copy link
Copy Markdown
Contributor

gretay-js commented May 11, 2023

What should happen for the annotation to check the zero-alloc property for all functions in a file? Do all functions in Lambda end up annotated with attributes as normal, or does a different flag need to be tested?

At the moment, there is a special flag !Clflags.zero_alloc_check_assert_all + a way to opt a function out by annotating it with an attribute Ignore_assert_all.

@mshinwell
Copy link
Copy Markdown
Collaborator Author

mshinwell commented May 11, 2023

What should happen for the annotation to check the zero-alloc property for all functions in a file? Do all functions in Lambda end up annotated with attributes as normal, or does a different flag need to be tested?

At the moment, there is a special flag !Clflags.zero_alloc_check_assert_all + a way to opt a function out by annotating it with an attribute Ignore_assert_all.

This PR should now handle that correctly.

@gretay-js
Copy link
Copy Markdown
Contributor

gretay-js commented May 11, 2023

could you please test to see if this suffices?

it works! I tested it on a bunch of functions that previously failed the check with Warning 199. Now they either pass or fail as expected. would it be hard to do the same for flambda?

@lthls
Copy link
Copy Markdown
Contributor

lthls commented May 12, 2023

could you please test to see if this suffices?

it works! I tested it on a bunch of functions that previously failed the check with Warning 199. Now they either pass or fail as expected. would it be hard to do the same for flambda?

It's not trivial with flambda1, because while flambda2 has actual functions stored separately from closures, in flambda1 they are stored together. So if we keep unused functions annotated with [@zero_alloc] then the closure will end up remaining and potentially causing an allocation in another function.
Here is an example:

let[@zero_alloc] f x =
  let[@zero_alloc][@inline] g y = x + y in
  g 1

If we keep g for checking, then f allocates the closure for it and fails the check.

@gretay-js
Copy link
Copy Markdown
Contributor

gretay-js commented May 12, 2023 •
edited
Loading

It's not trivial with flambda1 ..

makes sense, thanks.

@mshinwell mshinwell force-pushed the flambda2-zero-alloc-dont-delete branch from 9f083c7 to 9c46b05 Compare May 23, 2023 10:44
@mshinwell mshinwell linked an issue May 23, 2023 that may be closed by this pull request
Prevent deletion of functions with zero-alloc attributes #1341
Closed
lthls
lthls approved these changes May 25, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Approving the changes in simplify_set_of_closures.ml

@mshinwell mshinwell merged commit 373e70f into oxcaml:main May 25, 2023
mshinwell added a commit that referenced this pull request May 25, 2023
@mshinwell
Do not delete code of functions marked for zero-alloc check (#1378)
87cf349
@gretay-js gretay-js mentioned this pull request Nov 30, 2023
Merged
@gretay-js gretay-js mentioned this pull request Jul 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Gbury Gbury Gbury approved these changes

@lthls lthls lthls approved these changes

@poechsel poechsel Awaiting requested review from poechsel

@lukemaurer lukemaurer Awaiting requested review from lukemaurer

Assignees

No one assigned

Labels

flambda2 Prerequisite for, or part of, flambda2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Prevent deletion of functions with zero-alloc attributes Do not remove unused functions annotated with [@zero_alloc]

4 participants

@mshinwell @gretay-js @lthls @Gbury