Skip to content

Fix initialization of unboxed numbers in static blocks #2528

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 18 commits into from
May 7, 2024

Conversation

TheNumbat
Copy link
Contributor

@TheNumbat TheNumbat commented May 2, 2024

Flambda2 was incorrectly initializing unboxed static closure slots and static constants.

  • Closure slots were all assumed to be Word_val, which was interpreted as a pointer and hence passed to setfield. It would then call caml_initialize with a gc-unfriendly value, like an unboxed float. This also wouldn't fully copy a vec128.

  • Static constant boxed int64s and int64 arrays were incorrectly calling setfield with the unboxed int64 value due to using Word_int.

  • When Config.runtime5 was set, make_update unconditionally used setfield.

  • Naked vec128s would be written off the end of the closure block (this won't actually occur until unboxed vectors are exposed).

I found this in #2519, which re-enabled the assertion that we do not move between float & int registers. That was triggered when attempting to pass an unboxed float to caml_initialize.

@TheNumbat TheNumbat added the flambda2 Prerequisite for, or part of, flambda2 label May 2, 2024
@TheNumbat TheNumbat requested a review from mshinwell May 2, 2024 20:03
@TheNumbat TheNumbat added the bug Something isn't working label May 2, 2024
@TheNumbat
Copy link
Contributor Author

TheNumbat commented May 2, 2024

Note - writing an unboxed int32 into the closure environment currently needs to write 64 bits, because Project_value_slot loads Naked_int32s with the Word_int memory chunk instead of the sign-extending Thirtytwo_signed. This relies on the incoming int32 to already have been sign extended, which seems a bit dangerous.

I will instead make Project_value_slot sign extend (this is free on amd64).

@mshinwell
Copy link
Collaborator

I will discuss this with @TheNumbat

Gbury
Gbury previously requested changes May 3, 2024
@Gbury
Copy link
Contributor

Gbury commented May 3, 2024

Note - writing an unboxed int32 into the closure environment currently needs to write 64 bits, because Project_value_slot loads Naked_int32s with the Word_int memory chunk instead of the sign-extending Thirtytwo_signed. This relies on the incoming int32 to already have been sign extended, which seems a bit dangerous.

I will instead make Project_value_slot sign extend (this is free on amd64).

Well, this was written at a time where everything was a word, so this was reasonable and safe at that time. Clearly, with the advent of non-scannable slots in closures, as well as mixed blocks, we might want to change that, and indeed do sign-extending loads.

@TheNumbat
Copy link
Contributor Author

Sounds good; the change to memory_chunk_of_kind makes int32 loads sign-extend.

@TheNumbat
Copy link
Contributor Author

Added some more edits / explanations after discussing with @mshinwell

@mshinwell
Copy link
Collaborator

Please don't rebase this yet, as I'm still reviewing/editing.

@mshinwell
Copy link
Collaborator

@TheNumbat I've finished. I suggest looking at my new changes commit by commit, but read them all before commenting...

@mshinwell mshinwell dismissed Gbury’s stale review May 7, 2024 14:54

We've discussed and the appropriate changes have been made

@TheNumbat
Copy link
Contributor Author

New changes look good, merging

@TheNumbat TheNumbat merged commit 23b1314 into main May 7, 2024
15 checks passed
@TheNumbat TheNumbat deleted the fl2-unboxed-closure-init branch May 7, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working flambda2 Prerequisite for, or part of, flambda2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants