Head shape #9
Head shape #9
Conversation
nchataing
force-pushed
the
head_shape
branch
2 times, most recently
from
03d331d to
af30889
Compare
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
This is a first batch of comments, looking at the commits in order (I didn't get to the pattern-matching part yet). The split / history cleanup is very nice, thanks!
typing/typedecl_unboxed.ml
Outdated
| | Type_record (_, Record_inlined _) | ||
| | Type_record (_, Record_extension _) -> assert false | ||
| | Type_open -> block_shape [0] | ||
| | Type_variant ([],_) -> none |
There was a problem hiding this comment.
reading this code again, I find none a bit cryptic here; maybe empty would read better, even if the symmetry with any is broken?
There was a problem hiding this comment.
Maybe renaming both any to any_shape and none to empty_shape would read better.
There was a problem hiding this comment.
In general they will be used as Head_shape.{any,none,empty}, so it will read well. We happen to be inside the module so it's a bit more demanding on the naming.
There was a problem hiding this comment.
I think those values won't be in the signature of Head_shape so only to be used in the implementation.
toplevel/genprintval.ml
Outdated
| assert false | ||
| else if O.is_block obj | ||
| then Cstr_block(O.tag obj) | ||
| else Cstr_constant(O.obj obj) in |
There was a problem hiding this comment.
This genprintval.ml adaptation is tricky and will require more work. What the code is doing is to use type information to print a "raw value" in the toplevel. For boxed constructors, we use Obj.tag to get the tag at runtime, and then we query the constructor-descriptions to get the expected arity and type of the argument(s).
What should we do with unboxed constructor? What we need in general is a mapping from the tag (or immediate value) of the runtime value obj to one specific constructor, and to do this we need to inspect head shapes: if the value comes from an unboxed constructor, we can find it by comparing the runtime tag with its head shape.
Note: this does not work well for unboxed constructor whose argument is an empty type, if we allow several such constructors. But this is not a problem because such unboxed constructors are un-inhabited, the runtime value we have will never come from one of those (non-disjoint) unboxed constructors anyway.
There was a problem hiding this comment.
Discussing this again: things would work correctly if Datarepr.find_constr_by_tag was aware that the "raw tag" or "runtime tag" passed (maybe we should use a type different from constructor_tag; runtime_constructor_tag, without an unboxed case?) may not be one of the boxed tags, but it may also belong to the head shape of (at most one of) the unboxed constructors.
(We may need to force the head_shape here, instead of within Datarepr.)
There was a problem hiding this comment.
@nchataing remarks that this should be computed by matching not on the variant type declaration, but on the type description.
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
The end of my review (for head_shape as it was two days ago, if I understand correctly).
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
Thanks, the refactoring looks good.
I think that we are at the point where we could consider a "draft PR" against the main repository. (After cleaning up the history.) There are three remaining TODOs:
- adding some comments (I also asked for some extra comments in my last review): you should do this before submitting upstream
- handling of the
Record_extensioncase: no strong opinion, could be done before or after - trying the
Ctype.expand_optversion: I think we can keep it for after if you prefer
lambda/matching.ml
Outdated
| let sw_numconsts = | ||
| match Typedecl.cstr_max_imm_value pat_env cstr with | ||
| | Some n -> n + 1 | ||
| | None -> 1 (* XXX *) |
There was a problem hiding this comment.
What do we want here? max_imm_value is None when we have a Shape_any. In this case the any_const action is integrated into the fail_opt action above, so I would expect that we have no constant actions in the switch, suggesting sw_numconsts = 0.
There was a problem hiding this comment.
We discussed this. The problem is that simplif.ml assumes that if there is a fail-action, then one of the two cases is partial. With this code the assumption is wrong when fail_opt = None but any_const is not (so the "merged" fail_opt is Some), and we cover all nonconst cases.
So in theory we should put max_int here if there is an any_const, but this would blow up compilation later that creates an array of this size.
inline_lazy_force_switch has the same issue, there solution is to do a Pisint test first and then a switch (with sw_numconsts = 0, consts = []) only in the block case. We can do the same here:
- if
any_const = None, compile as in the current PR - if
fail_opt = Noneandany_const = Some, instead of defining a newfail_optwe should first generate aPisinttest, send to theany_constaction in the then branch, and generate a switch only in the else branch - if
fail_opt = Someandany_const = Some, we can either generate a Pisint first (as above) or generate code as in the current PR (probably generate nicer code).
The solution of using sw_numconsts = 1 may be unsafe (we are not sure) if the compiler then assumes that the immediate input may only be 0, not any immediate.
nchataing
force-pushed
the
head_shape
branch
2 times, most recently
from
5ceafe5 to
51f6664
Compare
…plementation (-> PR)
We introduce explicitly the "head" of a value; head shapes are finite approximations of sets of heads. We can compute the head of a value and check whether a head is included in a given head shape. This will be useful for toplevel/genprintval.ml which must dynamically decide the (possibly unboxed) constructor of a dynamic value of known type. TODO: we should discuss the placement of the types and operations, and document them; - maybe move the type of heads in types.mli - or maybe it's time to introduce dedicated compilation units, Head_shape_types (on which Types depends) and Head_shape (on which other units depend)?
(report from Guillaume Munch-Maccagnoni)
(suggestion from Guillaume Munch-Maccagnoni)
|
I see, my error was that I was thinking |
…s to [@extension.local] (#9) Attempts to make local allocations without -extension local are now caught at Lambda generation time.
The filename ça.ml is NFD normalized. The test source now the two normalizations for each unit identifiers.
a comment