Skip to content

Row_like indexes allows to represent a range rather than only upward closed set #361

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

Open
wants to merge 2 commits into
base: flambda2.0-stable
Choose a base branch
from

Conversation

chambart
Copy link

@chambart chambart commented Mar 22, 2021

This allows to more precisely represent the size of joins of polymorphic variants. This would be useful to be able to unbox something like:

let f b x =
  let v =
    if b then
      `A (x, x)
    else
      `B (x, 1, 2)
  in
  match v with
  | `A (a, b) -> a + b
  | `B (a, b, c) -> a + b + c

Notice that this is not enough to allow simplification of c to 2 here. Join still considers that joining between a known case and an absent case is equivalent to joining with top, where it should be equivalent to joining with bottom. We would need to refactor a bit to do that, which I defer for later.

In a first version the Range constructor took an option for the at_most field rather than having an At_least and Range case. But there is no factorisation to gain that way and the code is a bit cleaner like that.

This allows to more precisely represent the join of two known cases
| Range of { at_least : Index.t; at_most : Index.t; }
(** Range { at_least; at_most represents the set
{ x | at_least \subset x /\ x \subset at_most }
or { x | at_least \subset x } if at_most is None
Copy link

Choose a reason for hiding this comment

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

How can an index be None, from what I recall, Indexes can be integers at least in some cases...
Also, it's a bit weird to have this redundancy in how to represent at_least (i.e. either directly with At_least, or with Range using a None at_most), but no way to represent just the at_most case.

Copy link
Author

Choose a reason for hiding this comment

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

Oups I didn't update the comment. The case was

| Range of { at_least : Index.t; at_most : Index.t option; }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants