compiler: rewrite some legalizations, and remove a bunch of dead code #25772
Conversation
This file contains hidden or 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
mlugg
force-pushed
the
kill-dead-code
branch
3 times, most recently
from
30435f1 to
df0db0d
Compare
The main goal of this change was to avoid emitting the `vector_store_elem` AIR tag, because this represents an operation which Zig no longer supports (and hence Sema no longer emits) as of 010d9a6 (because runtime vector indices are now forbidden). Backends should not need to lower this operation, so I rewrote the legalizations which emitted it (scalarizations of vector operations) to instead unroll the loop and hence emit comptime-known vector indices. In doing this, I actually reworked those legalizations to use a different strategy; instead of using an `alloc` and storing to individual vector elements, the vector is constructed by-val, for instance by performing the scalar operation on all elements and passing them to an `aggregate_init`. This is vastly simpler to implement in Legalize, conceptually simpler, and doesn't severely pessimise memory usage, because a non-optimizing backend will store the full vector on the stack either way. Given the above rationale, I also ended up reworking several other legalizations to use simpler lowerings. The legalizations in question were bitcast scalarization, `struct_field_val` of `packed struct`s (where we just bitcast to an integer and perform the appropriate shift/trunc sequence), and `aggregate_init` of a `packed struct` (also implemented in terms of integer bitwise operations with bitcasts to and from the actual types). This hugely simplified some parts of `Legalize`. So, `Legalize` is now much simpler, and the `vector_store_elem` instruction is no longer emitted by any part of the compiler so can be removed in a future commit.
I started this diff trying to remove a little dead code from the C backend, but ended up finding a bunch of dead code sprinkled all over the place: * `packed` handling in the C backend which was made dead by `Legalize` * Representation of pointers to runtime-known vector indices * Handling for the `vector_store_elem` AIR instruction (now removed) * Old tuple handling from when they used the InternPool repr of structs * Straightforward unused functions * TODOs in the LLVM backend for features which Zig just does not support
`__addosi4`, `__addodi4`, `__addoti4`, `__subosi4`, `__subodi4`, and `__suboti4` were all functions which we invented for no apparent reason. Neither LLVM, nor GCC, nor the Zig compiler use these functions. It appears the functions were created in a kind of misunderstanding of an old language proposal; see ziglang#10824. There is no benefit to these functions existing; if a Zig compiler backend needs this operation, it is trivial to implement, and *far* simpler than calling a compiler-rt routine. Therefore, this commit deletes them. A small amount of that code was used by other parts of compiler-rt; the logic is trivial so has just been inlined where needed. I also chose to quickly implement `__addvdi3` (a standard function) because it is trivial and we already implement the `sub` parallel.
This isn't so much a regression as it is foreshadowing of accepted proposal ziglang#24657.
I had tried unrolling the loops to avoid requiring the `vector_store_elem` instruction, but it's arguably a problem to generate O(N) code for an operation on `@Vector(N, T)`. In addition, that lowering emitted a lot of `.aggregate_init` instructions, which is itself a quite difficult operation to codegen. This requires reintroducing runtime vector indexing internally. However, I've put it in a couple of instructions which are intended only for use by `Air.Legalize`, named `legalize_vec_elem_val` (like `array_elem_val`, but for indexing a vector with a runtime-known index) and `legalize_vec_store_elem` (like the old `vector_store_elem` instruction). These are explicitly documented as *not* being emitted by Sema, so need only be implemented by backends if they actually use an `Air.Legalize.Feature` which emits them (otherwise they can be marked as `unreachable`).
It turns out we did use these in the C backend. However, it's really just as easy, if not easier, to replicate the logic directly in C. Synchronizes stage1/zig.h to make sure the bootstrap doesn't depend on these functions either. The actual zig1 tarball is unmodified because regenerating it is unnecessary in this instance.
The changes to `codegen.c` are blatant hacks, but the problem they work around isn't a regression: it's an existing miscompilation. This branch happened to *expose* that miscompilation in more cases by changing how an incorrect result is *used*.
mlugg
force-pushed
the
kill-dead-code
branch
from
df0db0d to
532aa3c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
See commit messages.