compiler: rework comptime pointer representation and access #19630
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
These descriptions are always a joy to read if you are following along, thanks mlugg! |
mlugg
force-pushed
the
comptime-ptr-access-5
branch
4 times, most recently
from
f15a275
to
83e8d6d
Compare
mlugg
force-pushed
the
comptime-ptr-access-5
branch
4 times, most recently
from
4657609
to
1e2df59
Compare
Snektron
requested changes
Apr 13, 2024
mlugg
force-pushed
the
comptime-ptr-access-5
branch
4 times, most recently
from
40f4999
to
9a0131e
Compare
|
Okay, after correctly handling endianness, the new bitcast logic has become a bit messy. This is probably due some improvement, but since this PR is blocking 0.12 and actively blocking some major projects, I don't think that should be a PR blocker. |
mlugg
force-pushed
the
comptime-ptr-access-5
branch
3 times, most recently
from
332ec8c
to
6c55960
Compare
|
A pleasant surprise: I'm seeing zero significant performance deltas between master and this branch! I guess the comptime pointer access code just isn't hit often enough to make any difference, or perhaps the perf delta is just less than I thought. Build Behavior (x86_64)Note: this was tested on a common subset of behavior tests which pass on both Analyze CompilerBuild Compiler (x86_64) |
andrewrk
requested changes
Apr 16, 2024
There was a problem hiding this comment.
This implements a non-existing, non-accepted, breaking proposal to change the language semantics regarding loading values with partially undefined bits.
It is not clear whether these semantics are desired. A proposal that introduces a new kind of illegal behavior must address the question of how it will be safety-checked, if at all. If the proposal cannot provide a strategy for safety checking, it will probably be rejected, in which case this PR would introduce just as many regressions as it fixes.
I don't think this should be part of 0.12.0.
|
The commit I just pushed effectively un-implements #19634, and removes the workarounds in This decision can be revisited down the line: to either accept that proposal with some runtime safety, or to introduce tracking of partially-undefined values at comptime. |
mlugg
force-pushed
the
comptime-ptr-access-5
branch
2 times, most recently
from
5a4026e
to
1719ada
Compare
We've got a big one here! This commit reworks how we represent pointers in the InternPool, and rewrites the logic for loading and storing from them at comptime. Firstly, the pointer representation. Previously, pointers were represented in a highly structured manner: pointers to fields, array elements, etc, were explicitly represented. This works well for simple cases, but is quite difficult to handle in the cases of unusual reinterpretations, pointer casts, offsets, etc. Therefore, pointers are now represented in a more "flat" manner. For types without well-defined layouts -- such as comptime-only types, automatic-layout aggregates, and so on -- we still use this "hierarchical" structure. However, for types with well-defined layouts, we use a byte offset associated with the pointer. This allows the comptime pointer access logic to deal with reinterpreted pointers far more gracefully, because the "base address" of a pointer -- for instance a `field` -- is a single value which pointer accesses cannot exceed since the parent has undefined layout. This strategy is also more useful to most backends -- see the updated logic in `codegen.zig` and `codegen/llvm.zig`. For backends which do prefer a chain of field and elements accesses for lowering pointer values, such as SPIR-V, there is a helpful function in `Value` which creates a strategy to derive a pointer value using ideally only field and element accesses. This is actually more correct than the previous logic, since it correctly handles pointer casts which, after the dust has settled, end up referring exactly to an aggregate field or array element. In terms of the pointer access code, it has been rewritten from the ground up. The old logic had become rather a mess of special cases being added whenever bugs were hit, and was still riddled with bugs. The new logic was written to handle the "difficult" cases correctly, the most notable of which is restructuring of a comptime-only array (for instance, converting a `[3][2]comptime_int` to a `[2][3]comptime_int`. Currently, the logic for loading and storing work somewhat differently, but a future change will likely improve the loading logic to bring it more in line with the store strategy. As far as I can tell, the rewrite has fixed all bugs exposed by ziglang#19414. As a part of this, the comptime bitcast logic has also been rewritten. Previously, bitcasts simply worked by serializing the entire value into an in-memory buffer, then deserializing it. This strategy has two key weaknesses: pointers, and undefined values. Representations of these values at comptime cannot be easily serialized/deserialized whilst preserving data, which means many bitcasts would become runtime-known if pointers were involved, or would turn `undefined` values into `0xAA`. The new logic works by "flattening" the datastructure to be cast into a sequence of bit-packed atomic values, and then "unflattening" it; using serialization when necessary, but with special handling for `undefined` values and for pointers which align in virtual memory. The resulting code is definitely slower -- more on this later -- but it is correct. The pointer access and bitcast logic required some helper functions and types which are not generally useful elsewhere, so I opted to split them into separate files `Sema/comptime_ptr_access.zig` and `Sema/bitcast.zig`, with simple re-exports in `Sema.zig` for their small public APIs. Whilst working on this branch, I caught various unrelated bugs with transitive Sema errors, and with the handling of `undefined` values. These bugs have been fixed, and corresponding behavior test added. In terms of performance, I do anticipate that this commit will regress performance somewhat, because the new pointer access and bitcast logic is necessarily more complex. I have not yet taken performance measurements, but will do shortly, and post the results in this PR. If the performance regression is severe, I will do work to to optimize the new logic before merge. Resolves: ziglang#19452 Resolves: ziglang#19460
I have noted several latent bugs in the handling of packed memory on big-endian targets, and one of them has been exposed by the previous commit, triggering this test failure. I am opting to disable it for now on the ground that the commit preceding this one helps far more than it hurts.
This commit reverts the handling of partially-undefined values in bitcasting to transform these bits into an arbitrary numeric value, like happens on `master` today. As @andrewrk rightly points out, ziglang#19634 has unfortunate consequences for the standard library, and likely requires more thought. To avoid a major breaking change, it has been decided to revert this design decision for now, and make a more informed decision further down the line.
I have no idea why this wasn't being hit on master before.
The operation `undefined & 0` ought to result in the value `0`, and likewise for zeroing only some bits. `std/packed_int_array.zig` tests were failing because this behavior was not implemented -- this issue was previously masked by faulty bitcast logic which turned `undefined` values into `0xAA` on pointer loads. Ideally, we would like to be able to track the undefined bits at comptime. This is related to ziglang#19634.
mlugg
force-pushed
the
comptime-ptr-access-5
branch
from
1719ada
to
23062a5
Compare
|
Brilliant work. Thanks for that follow-up. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We've got a big one here! This commit reworks how we represent pointers in the InternPool, and rewrites the logic for loading and storing from them at comptime.
Firstly, the pointer representation. Previously, pointers were represented in a highly structured manner: pointers to fields, array elements, etc, were explicitly represented. This works well for simple cases, but is quite difficult to handle in the cases of unusual reinterpretations, pointer casts, offsets, etc. Therefore, pointers are now represented in a more "flat" manner. For types without well-defined layouts -- such as comptime-only types, automatic-layout aggregates, and so on -- we still use this "hierarchical" structure. However, for types with well-defined layouts, we use a byte offset associated with the pointer. This allows the comptime pointer access logic to deal with reinterpreted pointers far more gracefully, because the "base address" of a pointer -- for instance a
field-- is a single value which pointer accesses cannot exceed since the parent has undefined layout. This strategy is also more useful to most backends -- see the updated logic incodegen.zigandcodegen/llvm.zig. For backends which do prefer a chain of field and elements accesses for lowering pointer values, such as SPIR-V, there is a helpful function inValuewhich creates a strategy to derive a pointer value using ideally only field and element accesses. This is actually more correct than the previous logic, since it correctly handles pointer casts which, after the dust has settled, end up referring exactly to an aggregate field or array element.In terms of the pointer access code, it has been rewritten from the ground up. The old logic had become rather a mess of special cases being added whenever bugs were hit, and was still riddled with bugs. The new logic was written to handle the "difficult" cases correctly, the most notable of which is restructuring of a comptime-only array (for instance, converting a
[3][2]comptime_intto a[2][3]comptime_int. Currently, the logic for loading and storing work somewhat differently, but a future change will likely improve the loading logic to bring it more in line with the store strategy. As far as I can tell, the rewrite has fixed all bugs exposed by #19414.As a part of this, the comptime bitcast logic has also been rewritten. Previously, bitcasts simply worked by serializing the entire value into an in-memory buffer, then deserializing it. This strategy has two key weaknesses: pointers, and undefined values. Representations of these values at comptime cannot be easily serialized/deserialized whilst preserving data, which means many bitcasts would become runtime-known if pointers were involved, or would turn
undefinedvalues into0xAA. The new logic works by "flattening" the datastructure to be cast into a sequence of bit-packed atomic values, and then "unflattening" it; using serialization when necessary, but with special handling forundefinedvalues and for pointers which align in virtual memory. The resulting code is definitely slower -- more on this later -- but it is correct.The pointer access and bitcast logic required some helper functions and types which are not generally useful elsewhere, so I opted to split them into separate files
Sema/comptime_ptr_access.zigandSema/bitcast.zig, with simple re-exports inSema.zigfor their small public APIs.Whilst working on this branch, I caught various unrelated bugs with transitive Sema errors, and with the handling of
undefinedvalues. These bugs have been fixed, and corresponding behavior test added.In terms of performance, I do anticipate that this commit will regress performance somewhat, because the new pointer access and bitcast logic is necessarily more complex. I have not yet taken performance measurements, but will do shortly, and post the results in this PR. If the performance regression is severe, I will do work to to optimize the new logic before merge.
Resolves: #19452
Resolves: #19460