Skip to content

Allow immediate64s in mixed records#2589

Closed
ncik-roberts wants to merge 2 commits intomainfrom
nroberts/mixed-blocks-for-imm64
Closed

Allow immediate64s in mixed records#2589
ncik-roberts wants to merge 2 commits intomainfrom
nroberts/mixed-blocks-for-imm64

Conversation

@ncik-roberts
Copy link
Copy Markdown
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
Copy Markdown
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