Allow value slots (closure environment entries) to be of non-Value kind#924
Allow value slots (closure environment entries) to be of non-Value kind#924mshinwell wants to merge 1 commit intooxcaml:mainfrom
Conversation
|
It should be fairly easy: as it stand we already kind of do something similar with function slots and value slots: we first find offsets for all function slots first (starting with the smallest offsets), and then for all value slots, and we check that no function slots occurs after a value slot. So we would only need to extend that to work with function slots, then value slots of non-value kinds, and then value slots of kind value (and adapt the computation of the start of env, which would be really simple). |
|
I'm not well placed to review this, but it's encouraging to see the ease with which this was put together. Thanks, Mark! |
lthls
left a comment
There was a problem hiding this comment.
I've made a few comments. The main one is that I don't think the changes to the types directory are useful, as we already have kinds in the types themselves.
| index | ||
| | Project_value_slot { project_from = _; value_slot } -> | ||
| | Project_value_slot { project_from = _; value_slot; kind = _ } -> | ||
| (* CR mshinwell: could use [kind]? *) |
There was a problem hiding this comment.
It could be used either for a consistency check (that the kind of the result type is the expected one), or for adding subkind constraints. However, subkind constraints would better fit at the closure creation time, not on projection.
| Simple.pattern_match value_slot | ||
| ~const:(fun _ -> T.alias_type_of K.value value_slot) | ||
| ~const:(fun _ -> | ||
| T.alias_type_of K.value value_slot, kind_with_subkind) |
There was a problem hiding this comment.
| T.alias_type_of K.value value_slot, kind_with_subkind) | |
| T.alias_type_of kind value_slot, kind_with_subkind) |
| meet_generic_product env ~components_by_index1 ~components_by_index2 | ||
| ~union:Value_slot.Map.union | ||
| meet_generic_product env ~components_by_index1:components_by_index1' | ||
| ~components_by_index2:components_by_index2' ~union:Value_slot.Map.union |
There was a problem hiding this comment.
I think that instead of splitting off the kinds, you could have just defined a custom union:
let union f m1 m2 =
Value_slot.Map.union
(fun value_slot (ty1, kind1) (ty2, kind2) ->
if not (K.with_subkind.equal kind1 kind2) then Misc.fatal_errorf ...;
Option.map (fun ty -> ty, kind1) (f value_slot ty1 ty2))
m1 m2
in ...| let kind = | ||
| match component1, component2 with | ||
| | Some (_, kind1), Some (_, kind2) -> | ||
| (* CR mshinwell: is this too strong? *) |
There was a problem hiding this comment.
In theory, we could return Bottom too; but I think we can rely on the invariant that a given value slot can only hold values of a single kind (globally).
| { value_slot_components_by_index : t Value_slot.Map.t } | ||
| { value_slot_components_by_index : | ||
| (t * Flambda_kind.With_subkind.t) Value_slot.Map.t | ||
| } |
There was a problem hiding this comment.
It's not clear to me why it is useful to add the subkind here. The type already contains a kind.
There was a problem hiding this comment.
From Slack (from @lthls ): ... the layout computation only looks at the set of closure creations, so as long as we have full kinds and subkinds there we should be fine. If we started reifying closures it could be useful, but I'm not convinced it would be necessary even in this case
|
This idea makes sense to me! The runtime should be OK with putting non-value data in the code pointer section of closures, except for this part of the compactor - I think that's the only spot where the runtime iterates over the infix code pointers of a closure. (This shouldn't be hard to change, but will need a bit of work. For development, you can just test with compaction disabled) |
|
@stedolan is proposing to have a magic word prior to the unboxed part of the environment, which can be used for compaction. So from the middle end point of view it's very similar. |
|
I thought about the compactor problem a bit more. Two suggestions:
|
I quickly sketched this out to see whether there were likely to be any problems inside Flambda 2; it seems ok. We would need something like this for parts of the forthcoming unboxed types proposal (e.g. naked floats being passed to functions, which could cause them to appear inside closures after partial applications).
I have an idea for the closure representation part of this which avoids changing the runtime. It involves segregating value slots into ones of
Valuekind, and everything else. This is easy now that the Flambda 2 terms give the kinds. Then the non-Valueones would be put in fields after the last code pointers, immediately prior to thestartenvpoint. I think this will avoid disturbing the placement of any code pointers, yet also conceal these non-scannable fields from the GC. (We could also, indeed, put any value slots known to be of kindValuebut subkindTagged_immediatein the non-scannable area.)So the question is @Gbury : how difficult would it be to adapt
Slot_offsetsfor such a layout?This would be needed for Closure and Flambda 1 too.