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

AstGen: correctly deduplicate ref of param and alloc_inferred #22164

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

mlugg
Copy link
Member

@mlugg mlugg commented Dec 6, 2024

Both of these instructions were previously under a special case in rvalue which resulted in every reference to such an instruction adding a new ref instruction. This had the effect that, for instance, &a != &a for parameters. Deduplicating these ref instructions was problematic for different reasons.

For alloc_inferred, the problem was that it's not valid to ref the alloc until the allocation has been resolved (resolve_inferred_alloc), but AstGen.appendBodyWithFixups would place the ref directly after the alloc_inferred. This is solved by bringing resolve_inferred_alloc in line with make_ptr_const by having it return the final pointer, rather than modifying sema.inst_map of the original alloc_inferred. That way, the ref refers to the resolve_inferred_alloc instruction, so is placed immediately after it, avoiding this issue.

For param, the problem is a bit trickier: param instructions live in a body which must contain only param instructions, then a func{,_inferred,_fancy}, then a break_inline. Moreover, param instructions may be referenced not only by the function body, but also by other parameters, the return type expression, etc. Each of these bodies requires separate ref instructions. This is solved by pulling entries out of ref_table after evaluating each component of the function declaration, and appending the refs later on when actually putting the bodies together. This gives way to another issue: if you write fn f(x: T) @TypeOf(x.foo()), then since x.foo() takes a reference to x, this ref instruction is now in a comptime context (outside of the @TypeOf ZIR body), so emits a compile error. This is solved by loosening the rules around ref instructions; because they are not side-effecting, it is okay to allow ref of runtime values at comptime, resulting in a runtime-known value in a comptime scope. We already apply this mechanism in some cases; for instance, it's why runtime_array.len works in a comptime context. In future, we will want to give similar treatment to many operations in Sema: in general, it's fine to apply runtime operations at comptime provided they don't have side effects!

Resolves: #22140

@mlugg mlugg force-pushed the astgen-ref-dedup branch 2 times, most recently from 0b88d4b to c5734ec Compare December 6, 2024 11:32
@xdBronch
Copy link
Contributor

xdBronch commented Dec 6, 2024

does this solve #16343 and/or #18194?

@mlugg
Copy link
Member Author

mlugg commented Dec 7, 2024

@jacobly0 thanks for that fix, beat me to adding -Wno-tautological-compare :D

@mlugg
Copy link
Member Author

mlugg commented Dec 7, 2024

@xdBronch #16343 no, that's a more complex issue related to PRO. #18194 yes, that's actually a duplicate (duplicate-ee?) -- thanks!

mlugg and others added 3 commits December 8, 2024 10:53
Both of these instructions were previously under a special case in
`rvalue` which resulted in every reference to such an instruction adding
a new `ref` instruction. This had the effect that, for instance,
`&a != &a` for parameters. Deduplicating these `ref` instructions was
problematic for different reasons.

For `alloc_inferred`, the problem was that it's not valid to `ref` the
alloc until the allocation has been resolved (`resolve_inferred_alloc`),
but `AstGen.appendBodyWithFixups` would place the `ref` directly after
the `alloc_inferred`. This is solved by bringing
`resolve_inferred_alloc` in line with `make_ptr_const` by having it
*return* the final pointer, rather than modifying `sema.inst_map` of the
original `alloc_inferred`. That way, the `ref` refers to the
`resolve_inferred_alloc` instruction, so is placed immediately after it,
avoiding this issue.

For `param`, the problem is a bit trickier: `param` instructions live in
a body which must contain only `param` instructions, then a
`func{,_inferred,_fancy}`, then a `break_inline`. Moreover, `param`
instructions may be referenced not only by the function body, but also
by other parameters, the return type expression, etc. Each of these
bodies requires separate `ref` instructions. This is solved by pulling
entries out of `ref_table` after evaluating each component of the
function declaration, and appending the refs later on when actually
putting the bodies together. This gives way to another issue: if you
write `fn f(x: T) @typeof(x.foo())`, then since `x.foo()` takes a
reference to `x`, this `ref` instruction is now in a comptime context
(outside of the `@TypeOf` ZIR body), so emits a compile error. This is
solved by loosening the rules around `ref` instructions; because they
are not side-effecting, it is okay to allow `ref` of runtime values at
comptime, resulting in a runtime-known value in a comptime scope. We
already apply this mechanism in some cases; for instance, it's why
`runtime_array.len` works in a `comptime` context. In future, we will
want to give similar treatment to many operations in Sema: in general,
it's fine to apply runtime operations at comptime provided they don't
have side effects!

Resolves: ziglang#22140
When a shard has zero elements, we don't need to reserve any capacity.
@andrewrk andrewrk merged commit 8245d7f into ziglang:master Dec 9, 2024
10 checks passed
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.

&arr make a copy of the array
4 participants