Skip to content

More thorough treatment of closure offsets#496

Merged
mshinwell merged 11 commits intooxcaml:mainfrom
Gbury:used_closure_elts
Feb 9, 2022
Merged

More thorough treatment of closure offsets#496
mshinwell merged 11 commits intooxcaml:mainfrom
Gbury:used_closure_elts

Conversation

@Gbury
Copy link
Copy Markdown
Contributor

@Gbury Gbury commented Feb 1, 2022

It can happen that a closure_id/closure_var from another compilation unit only occur in types in the current compilation unit (mainly due to functions/functor's result types), and in that case, we need to export the offsets for these ids/vars, so that later units have access to these offsets without the original compilation unit's cmx.

@Gbury Gbury requested review from lthls and mshinwell as code owners February 1, 2022 13:46
@mshinwell mshinwell added bug Something isn't working flambda2 Prerequisite for, or part of, flambda2 flambda2 beta labels Feb 1, 2022
@lthls lthls mentioned this pull request Feb 3, 2022
Merged
@Gbury Gbury force-pushed the used_closure_elts branch 2 times, most recently from e941989 to a3412de Compare February 4, 2022 15:46
@Gbury Gbury requested a review from chambart as a code owner February 4, 2022 15:46
@Gbury Gbury force-pushed the used_closure_elts branch from a3412de to 7a2b4ce Compare February 4, 2022 16:28
Gbury added 7 commits February 7, 2022 15:06
@Gbury
Properly count closure_vars in metadata free names
a316ac7
@Gbury
Export offsets for ids/vars that occur in types
23699b7 
It can happen that a closure_id/closure_var from another compilation
unit only occur in types in the current compilation unit (mainly due to
functions/functor's result types), and in that case, we need to export
the offsets for these ids/vars, so that later units have access to these
offsets without the original compilation unit's cmx.
@Gbury
Better criterion for used closure vars
2956a1c
@Gbury
Be more explicit about closure vars that are removed
32cfdef
@Gbury
Add in exported offsets the closure elts in the typing env
0781e61
@Gbury
Fail at compile-time on missing offsets
ead43bb
@Gbury
Remove stale CR
9eeee8c
@Gbury Gbury force-pushed the used_closure_elts branch from cf7c502 to 9eeee8c Compare February 7, 2022 14:10
@Gbury Gbury changed the title Export offsets for ids/vars that occur in types More thorough treatment of closure offsets Feb 7, 2022
@Gbury
Copy link
Copy Markdown
Contributor Author

Gbury commented Feb 7, 2022

This PR has evolved quite a bit, so here is what it does now:

  • record not only the offset for closure_vars/closure_ids, but also record the fact that we voluntarily do not assign offset to some closure_vars/ids that are considered dead
  • for closure_vars/ids from the current compilation unit:
    • we assign offsets to all closure_ids that appear in set of closures creation
    • we assign offsets for all closure_vars that occur both in projections at normal name mode, and in creation of sets of closures at normal name mode
    • closure_ids and vars that are not assigned offsets, but occur in projections or sets of closure creations are assigned a dead_var/id offset information.
    • we use the set of closures to which we have assigned an actual offset to then clean the exported typing env
  • for the closure_vars/ids from another compilation unit:
    • we fetch their current offset info (either the actual offset, or the deaf_var/id info), and re-export that for the cmx of the current compilation unit
    • we throw a fatal error if a closure_var/id has no offset info
  • as a consequence, to_cmm no longer needs access to the set of used_closure_vars since it can directly look up the info in the exported_offsets.

lthls
lthls approved these changes Feb 9, 2022
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.

I believe that the code is correct. I've left quite a few comments in places where I think the code could be improved.
There's an additional comment that I couldn't place as it occurs in code that isn't part of the diff: Exported_offsets.add_closure_offset and Exported_offsets.add_env_var_offset use the polymorphic comparison in an assertion, and I think that should be fixed too.

lthls
lthls reviewed Feb 9, 2022
View reviewed changes
@Gbury @lthls
Update middle_end/flambda2/simplify_shared/exported_offsets.ml
d456f89 
Co-authored-by: Vincent Laviron <vincent.laviron@gmail.com>
@mshinwell mshinwell merged commit e3b8450 into oxcaml:main Feb 9, 2022
mshinwell pushed a commit that referenced this pull request Mar 3, 2022
@Gbury @mshinwell
More thorough treatment of closure offsets (#496)
4128b3a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lthls lthls lthls approved these changes

@mshinwell mshinwell mshinwell approved these changes

@chambart chambart Awaiting requested review from chambart

Assignees

No one assigned

Labels

bug Something isn't working flambda2 beta flambda2 Prerequisite for, or part of, flambda2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Gbury @mshinwell @lthls