Skip to content

Unboxed ints (int32, int64, and nativeint) #1750

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

Closed
wants to merge 43 commits into from

Conversation

antalsz
Copy link
Contributor

@antalsz antalsz commented Aug 16, 2023

This adds unboxed integer support in the same vein as unboxed float support:

  • int32#, int64#, and nativeint# types
  • bits32, bits64, and word layouts
  • Stdlib__Int32_u, Stdlib__Int64_u, and Stdlib__Nativeint_u modules

There's one standing issue: unboxed integers are still sometimes allocating with the closure and flambda middle-ends, but not the flambda2 middle-end, due to some inling failure. I'll be investigating that, but the rest should be reviewable.

Reviwers: @goldfirere or @ccasin

@mshinwell
Copy link
Collaborator

I'll read everything from Lambda (including Translprim) onwards.

Copy link
Collaborator

@goldfirere goldfirere left a comment

Choose a reason for hiding this comment

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

This is a partial review, but I've now timed out. (I've gone through files in the order presented on GitHub, skipping files that @mshinwell has offered to review. Stopped at unboxed_bits32s_alpha.ml.) I may be able to take a further look during the week, but it's more likely I will return to this in a week's time. @ccasin may take over.

@antalsz
Copy link
Contributor Author

antalsz commented Sep 1, 2023

Incorrect allocation fixed, but I still need to respond to review

Copy link
Contributor

@ccasin ccasin left a comment

Choose a reason for hiding this comment

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

This looks good! I read everything except the backend changes (which @mshinwell has said he will look at). Though I only skimmed the bits Richard had already reviewed (and while I tried to read all of the tests, I'm not claiming I really thought hard about the details of the three quickcheck implementations).

I'm holding off on clicking the approve button so we don't accidentally merge with the XXXs in there, but my comments are all minor.

A couple additional thoughts:

  1. As observed here and elsewhere, the duplication of the tests is a bit sad. Of course, not testing things is also a bit sad. We can divide the tests into a couple categories:
    • For the runtime tests, I think having the copies is basically mandatory - this guards against copy-paste errors in the backend.
    • For the typing tests, it's harder to imagine such an error now, considering how small the actual diff in typing is here. But it's conceivable we'll decide to do different things with these layouts in the future (special treatment for small ints in arrays, for example). I think on balance I like having them, with a plan to replace them with some functor over abstract layouts in the far future. That will be a nice day.
  2. Should this just go straight into layouts_beta? I think we've basically decided to stop using alpha for "normal" new layouts things, and reserve it for experimental or unusually risky things, and I suspect this falls into the former category. (If you agree, rather than copying things into the new beta tests, better to do what's done in the latest version of the float tests where there's one test file and the TEST stanzas just run the test at both flags, for tests expected to behave the same)

(* XXX ASZ: What layouts version does this go with? *)
(* CR layouts XXX ASZ: Change [typ_int32] to fit in a 32-bit register, rather
than taking up a full 64-bit one. *)
let typ_int64 = if Arch.size_int = 4 then [|Int;Int|] else [|Int|]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having the complication of thinking about these multi-element arrays, I would just assert that Arch.size_int is 8. It is very unlikely it will ever be 4 again.

Copy link
Collaborator

@mshinwell mshinwell left a comment

Choose a reason for hiding this comment

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

I've only looked at backend/ and lambda/.

@ccasin
Copy link
Contributor

ccasin commented Dec 6, 2023

Now #2113

@ccasin ccasin closed this Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants