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.

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

Static allocation for mixed blocks etc. #2712

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

mshinwell
Copy link
Collaborator

This is based on #2692 although for review purposes, it should be read from 0d24f6a7bc9fff5bd14bccc1b97304c73b9474ed as a whole. It should provide full support for mixed block static allocation in both classic and optimized modes.

One nice thing is as follows: we can ditch the old Field_of_static_block type entirely and just use a Simple paired with a Debuginfo. In addition we can just have a single constructor for scannable blocks in Static_const.

The types have been carefully chosen with regard to the Scannable submodules so the encoding should be precise. (Some of the block shape types introduced in #2533 have been reworked a little to make this all fit together nicely.) The block shapes being carried along with the lists of Simples used to build a statically-allocated block provide all the necessary information required for compilation to Cmm.

Since it was useful for getting the types right I've also added support in classic mode for statically allocating all-float blocks (note these are blocks, e.g. from records, not arrays so the float array optimization isn't relevant here: they are always stored unboxed). There are no approximations for these however.

In addition there is a commit which introduces a new type to describe elements of a mixed block's flat suffix. I think we should avoid hanging onto Lambda types into Flambda 2: separate types for separate IRs enable nuances to be encoded more effectively, amongst other things. Here there is a concrete example: the Float_boxed constructor doesn't seem to make sense at Cmm time in the existing Lambda type that is being used for flat suffix elements, since any boxing required would have been expanded on the way into Flambda 2.

I need to do another pass to make sure everything looks ok and tests pass, but I'm putting this up now to gather preliminary feedback.

@mshinwell mshinwell added flambda2 Prerequisite for, or part of, flambda2 unboxed types labels Jun 24, 2024
middle_end/flambda2/to_cmm/to_cmm_primitive.ml Outdated Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_static.ml Outdated Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_static.ml Outdated Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_static.ml Show resolved Hide resolved
@mshinwell mshinwell linked an issue Jun 25, 2024 that may be closed by this pull request
@mshinwell
Copy link
Collaborator Author

@lthls is going to review this.

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.

I'm fine with the overall approach and implementation. I've left a number of comments in places where I think the code could be easily improved.

middle_end/flambda2/from_lambda/closure_conversion.ml Outdated Show resolved Hide resolved
middle_end/flambda2/kinds/flambda_kind.ml Outdated Show resolved Hide resolved
middle_end/flambda2/kinds/flambda_kind.ml Outdated Show resolved Hide resolved
middle_end/flambda2/simplify/rebuilt_static_const.ml Outdated Show resolved Hide resolved
middle_end/flambda2/simplify/rebuilt_static_const.ml Outdated Show resolved Hide resolved
middle_end/flambda2/term_basics/simple.ml Outdated Show resolved Hide resolved
middle_end/flambda2/term_basics/simple.ml Outdated Show resolved Hide resolved
middle_end/flambda2/terms/flambda_primitive.ml Outdated Show resolved Hide resolved
middle_end/flambda2/types/reify.ml Outdated Show resolved Hide resolved
middle_end/flambda2/types/reify.ml Outdated Show resolved Hide resolved
@lthls
Copy link
Contributor

lthls commented Jul 9, 2024

I've found a few bugs remaining in this PR that will need to be addressed. One is a variant of #2763 (To_cmm_static.static_field needs to be updated so that all expressions have the correct size), and I also suspect a bug in the translation of constant mixed blocks, where the code in Closure_conversion fails to properly unbox the constants that have kind Float_boxed. Hopefully fixing both of those should be enough to make the testsuite go green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flambda2 Prerequisite for, or part of, flambda2 unboxed types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static allocation for mixed blocks
4 participants