Skip to content
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

Introduce std.mem.readPackedInt and improve bitcasting of packed memory layouts #13221

Merged
merged 5 commits into from
Oct 29, 2022

Conversation

topolarity
Copy link
Contributor

Packed memory has a well-defined layout that doesn't require actually using a BigInt to read from. Let's use it :-)

Included changes:

  • Adds readPackedInt, writePackedInt and friends to std.mem
  • Adds support for casting to packed enums and vectors
  • Fixes bitcasting to/from vectors outside of a packed struct
  • Adds a fast path for bitcasting <= u/i64
  • Fixes bug when bitcasting f80 which would clear following fields

@topolarity topolarity changed the title Introduce std.mem.readPackedInt and improve bitcasting of packed memory layouts. Introduce std.mem.readPackedInt and improve bitcasting of packed memory layouts Oct 18, 2022
lib/std/mem.zig Outdated Show resolved Hide resolved
Copy link
Contributor

@matu3ba matu3ba left a comment

Choose a reason for hiding this comment

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

Very dense math stuff. I wonder, what the perf results will be.

So far writePackedIntLittle and writePackedIntBig have no test coverage testing them against another and it would be nice to reverse the ints with byteswap to ensure both work as expected. At least, if that is feasible.

lib/std/mem.zig Outdated Show resolved Hide resolved
lib/std/mem.zig Outdated Show resolved Hide resolved
lib/std/mem.zig Show resolved Hide resolved
lib/std/mem.zig Outdated Show resolved Hide resolved
lib/std/mem.zig Show resolved Hide resolved
src/value.zig Outdated Show resolved Hide resolved
src/value.zig Show resolved Hide resolved
src/value.zig Show resolved Hide resolved
@topolarity
Copy link
Contributor Author

So far writePackedIntLittle and writePackedIntBig have no test coverage testing them against another and it would be nice to reverse the ints with byteswap to ensure both work as expected. At least, if that is feasible.

I think this is already covered. writePackedIntLittle/Big are tested through writePackedInt, including foreign endianness with byte swaps.

random.int(BackingType), // random
}) |init_value| {
const uTs = [_]type{ u1, u3, u7, u8, u9, u10, u15, u16, u86 };
const iTs = [_]type{ i1, i3, i7, i8, i9, i10, i15, i16, i86 };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brief call-out: The codegen on this test is absurd.

Parts of this inline for are generated 18 * 4 * 3 = 216 times. It was vital while developing, but we may want to disable it or pare down the tested combinations now that we know these functions work.

Copy link
Member

@andrewrk andrewrk 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 like an important changeset. The first thing I started looking at is the changed behavior tests, and I noticed that it links to a closed issue. So I'm guessing that this PR needs to be revisited before it is ready for review-

test/behavior/bitcast.zig Outdated Show resolved Hide resolved
These utility functions allow reading from (stage2) packed memory at
runtime-known offsets.
Packed memory has a well-defined layout that doesn't require
conversion from an integer to read from. Let's use it :-)

This change means that for bitcasting to/from a packed value that
is N layers deep, we no longer have to create N temporary big-ints
and perform N copies.

Other miscellaneous improvements:
  - Adds support for casting to packed enums and vectors
  - Fixes bitcasting to/from vectors outside of a packed struct
  - Adds a fast path for bitcasting <= u/i64
  - Fixes bug when bitcasting f80 which would clear following fields

This also changes the bitcast memory layout of exotic integers on
big-endian systems to match what's empirically observed on our targets.
Technically, this layout is not guaranteed by LLVM so we should probably
ban bitcasts that reveal these padding bits, but for now this is an
improvement.
@andrewrk andrewrk merged commit c36eb4e into ziglang:master Oct 29, 2022
@topolarity topolarity deleted the packed-mem branch November 1, 2022 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants