Skip to content

Allow immediate64s in mixed records #2589

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 2 commits into from

Conversation

ncik-roberts
Copy link
Contributor

@ncik-roberts ncik-roberts commented May 17, 2024

Allow immediate64s in the non-value suffix of mixed records. On its face, this may seem concerning, because immediate64s are boxed on 32 bit platforms. But this is nonetheless OK because mixed records are only represented as mixed blocks (with a flat, non-scannable suffix) on 64 bit platforms.

Changes to the code:

  • add Imm64 as an allowed element of the flat suffix
  • as a driveby, rename the Float constructor of flat_element to Float_boxed per @mshinwell's suggestion. The old name is easy to confuse with Float32 and Float64. (This change touches a similar set of places in code.)

We have to propagate the immediate64-ness of the field up until lambda. This is point that makes the call as to whether the value is boxed. Assumption: once we hit flambda2, we know that immediate64s are immediates — I believe this assumption is already present in the code base, but I'm less familiar with this part of the world, so I want to flag it explicitly to the flambda2 reviewer.

Changes to auto-generated tests:

  • I kick the tests to make them cycle through all of the flat element kinds more eagerly
  • I also delete a bunch of repetitive/unnecessary generated tests, and reduce the number of constructor arg tests
    There are also normal unit tests.

I suggest that @ccasin reviews (and I'll get someone else to take a look at the flambda2 pieces afterward). This is non-urgent to review.

and it makes the auto-gen'ed code a bit more compact to read anyway.
@ncik-roberts ncik-roberts force-pushed the nroberts/mixed-blocks-for-imm64 branch from aa93109 to c73a1a4 Compare May 17, 2024 13:10
@ncik-roberts
Copy link
Contributor Author

After some discussion with Mark, we're going to put this on hold until we're more sure whether or not we'll ever want flambda2 to support unboxing on 32 bit platforms. (It's possible, but not clear one way or another.)

Accepting a mixed record definition requires that, if the runtime rep is a real mixed block with a flat suffix, then everything in the flat suffix must be stored flat. We want the same set of mixed record definitions to be accepted for 32 and 64 bit backends. So, if flambda2 starts supporting 32 bit platforms with unboxing, we'd regret ever allowing immediate64s in the flat suffix.

This isn't a big limitation for users, as they can just move the immediate64s into the value prefix. (The GC will scan them, but we could probably change this on 64 bit platforms if the immediate64 fields are put right before the flat suffix.)

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.

1 participant