Skip to content

Extend Pblock value kind to handle variants #703

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

Merged
merged 1 commit into from
Jul 5, 2022

Conversation

mshinwell
Copy link
Collaborator

This replaces the existing Pblock value kind used in Lambda with a Pvariant kind. This allows Flambda 2 to have more precise types for function parameters.

I'll tidy up the printing functions after any review comments have been received on the remainder.

@mshinwell mshinwell added flambda2 Prerequisite for, or part of, flambda2 typing labels Jun 27, 2022
@mshinwell
Copy link
Collaborator Author

One thing that may need discussion is the "fuel" setting, by the way.

@mshinwell mshinwell force-pushed the lambda-variant-kind branch 3 times, most recently from 80b7086 to 5dda254 Compare June 28, 2022 08:42
Copy link
Collaborator

@lpw25 lpw25 left a comment

Choose a reason for hiding this comment

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

The changes to typeopt look correct.

However, I'm worried about the size of data you might be creating here. There is no sharing between different occurences of the same type, and it would not take that many constructors with that many fields before the size becomes very large. In particular, you are now going to get blocks for recursive types, so you are much more likely to see a large type nested immediately inside another large type. I would suggest that the fuel reduction on each iteration should take account of the number of constructors, and probably the number of fields that a type has.

@mshinwell
Copy link
Collaborator Author

Yeah, I will add some aggressive fuel reduction. I think we should probably forbid Pvariant from representing Pintval.

@lthls
Copy link
Contributor

lthls commented Jul 1, 2022

I think we should probably forbid Pvariant from representing Pintval.

In theory we would have more precise information with a const-only Pvariant than with Pintval. I can't think of anything useful we could do with that extra information though (at least without Obj.magic), so I'd also go for forbidding Pvariant with an empty non_consts.

@mshinwell mshinwell force-pushed the lambda-variant-kind branch from 5dda254 to 812e40e Compare July 4, 2022 15:23
@mshinwell
Copy link
Collaborator Author

I've added a new fuel algorithm, comments welcome (particularly on the setting of the various fuel usage parameters).

@mshinwell
Copy link
Collaborator Author

(Test failures will be due to Lambda IR output changing, will fix once we are all in agreement.)

Copy link
Collaborator

@lpw25 lpw25 left a comment

Choose a reason for hiding this comment

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

Fuel check looks good now

@mshinwell mshinwell force-pushed the lambda-variant-kind branch from 5af09b4 to 211276c Compare July 5, 2022 13:52
@mshinwell mshinwell merged commit fb4a897 into ocaml-flambda:main Jul 5, 2022
ccasin added a commit that referenced this pull request Jul 26, 2022
ce76e02 flambda-backend: Bugfix for type_application (#746)
44f3afb flambda-backend: PR580 for main branch (#743)
b851eaa flambda-backend: Backport first part of ocaml/ocaml PR10498 (#737)
fafb4bd flambda-backend: Fix return mode for eta-expanded function in type_argument (#735)
c31f6c3 flambda-backend: Fix treatment of functions called [@nontail] (#725)
847781e flambda-backend: Fix build_upstream post-PR703 (#712)
bfcbbf8 flambda-backend: Extend Pblock value kind to handle variants (#703)
b2cab95 flambda-backend: Merge ocaml-jst
a6d6e0e flambda-backend: Merge ocaml-jst
88a4f63 flambda-backend: Use Pmakearray for immutable arrays (#699)
eeaa44b flambda-backend: Install an ocamldoc binary (#695)
48d322b flambda-backend: Ensure that GC is not invoked from bounds check failures (#681)
4370fa1 flambda-backend: Review changes of term directory (#602)
65a4566 flambda-backend: Add code coverage using bisect_ppx (#352)
63ab65f flambda-backend: Bugfix for primitive inclusion (#662)
7e3e0c8 flambda-backend: Fix inclusion checks for primitives (#661)
96c68f9 flambda-backend: Speed up linking by changing cmxa format (#607)
1829150 flambda-backend: Bugfix for Translmod.all_idents (#659)

git-subtree-dir: ocaml
git-subtree-split: ce76e02
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 typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants