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

special-case the lowering of ... catch(err) switch(err) and if (...) |p| ... else |err| switch(err) #11957

Closed
andrewrk opened this issue Jun 29, 2022 · 1 comment · Fixed by #18173
Labels
accepted This proposal is planned. enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

This proposal is mainly concerned about the self-hosted compiler implementation, however it also has some implications for the language specification, discussed at the end.

The idea here is to special-case switching on an error, and lower it in a more compact way:

test {
    var a: error{ A, B }!u64 = 0;
    var b = a catch |err| switch (err) {
        error.A => 0,
        else => unreachable,
    };
}

Status quo ZIR looks something like this. Note the double block; one for the catch and one for the switch:

 %39 = block({
   %36 = load(%31) node_offset:10:13
   %37 = is_non_err(%36) node_offset:10:15
   %38 = condbr(%37, {
     %40 = err_union_payload_unsafe(%36) node_offset:10:15
     %56 = break(%39, %40)
   }, {
     %42 = err_union_code(%36) node_offset:10:15
     %43 = switch_cond(%42) node_offset:10:35
     %44 = typeof(%43) node_offset:10:35
     %46 = error_value("A") token_offset:11:15
     %47 = as_node(%44, %46) node_offset:11:9
     %45 = switch_block(%43,
       else => {
         %52 = dbg_block_begin())
         %54 = dbg_block_end())
         %53 = unreachable(node_offset:12:17
       },
       %47 => {
         %48 = dbg_block_begin())
         %50 = dbg_block_end())
         %51 = break(%45, @Zir.Inst.Ref.zero)
       }) node_offset:10:27
     %57 = break(%39, %45)

After this proposal it would be one block whose operand is the error union directly. The "ok" prong is for the non-error case.

 %36 = load(%31) node_offset:10:13
 %45 = switch_block_err_union(%36,
   ok => {
    %40 = err_union_payload_unsafe(%36) node_offset:10:15
    %a = break(%45, %40)
   },
   else => {
     %53 = unreachable() node_offset:12:17
   },
   %47 => {
     %51 = break(%45, @Zir.Inst.Ref.zero)
   }) node_offset:10:27
 %57 = break(%39, %45)

That example is specific to the pattern catch |err| switch (err)

If-error syntax could additionally take advantage of this:

test {
    var a: error{ A, B }!u64 = 0;
    var b = if (a) |x| x else |err| switch (err) {
        error.A => 0,
        else => unreachable,
    };
}

This would lower to the exact same ZIR.

Language Specification Implications

In both of these code examples, this would make the success expression and the error expressions peers. This has implications for peer type resolution as well as result location coercion. It prevents this compile error:

../test/behavior/error.zig:711:37: error: value with comptime only type 'comptime_int' depends on runtime control flow
    var value = S.foo() catch |err| switch (err) {
                                    ^
../test/behavior/error.zig:711:45: note: runtime control flow here
    var value = S.foo() catch |err| switch (err) {
                                            ^

because the success expression participates in the type resolution, and the error expressions end up coercing to a type capable of storing a runtime value.

This is the main motivation for this proposal.

Additional Benefits

This pattern is extremely common in Zig source code, and this change reduces the amount of data passing through most phases of the compiler pipeline, which should be a small performance benefit.

Additionally, the respective AIR code will contain a single switch_br rather than a cond_br and a switch_br, likely resulting in improved machine code of debug builds.

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Jun 29, 2022
@andrewrk andrewrk added this to the 0.11.0 milestone Jun 29, 2022
andrewrk added a commit that referenced this issue Jun 29, 2022
This reverts commit 8bf3e1f, which
introduced miscompilations for peer expressions any time they needed
coercions to runtime types.

I opened #11957 as a proposal to accomplish the goal of the reverted
commit.

Closes #11898
andrewrk added a commit that referenced this issue Jun 29, 2022
This reverts commit 8bf3e1f, which
introduced miscompilations for peer expressions any time they needed
coercions to runtime types.

I opened #11957 as a proposal to accomplish the goal of the reverted
commit.

Closes #11898
@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. accepted This proposal is planned. frontend Tokenization, parsing, AstGen, Sema, and Liveness. labels Jun 29, 2022
andrewrk added a commit that referenced this issue Jul 19, 2022
This reverts commit 8bf3e1f, which
introduced miscompilations for peer expressions any time they needed
coercions to runtime types.

I opened #11957 as a proposal to accomplish the goal of the reverted
commit.

Closes #11898
wooster0 pushed a commit to wooster0/zig that referenced this issue Jul 24, 2022
This reverts commit 8bf3e1f, which
introduced miscompilations for peer expressions any time they needed
coercions to runtime types.

I opened ziglang#11957 as a proposal to accomplish the goal of the reverted
commit.

Closes ziglang#11898
@andrewrk andrewrk modified the milestones: 0.11.0, 0.12.0 Apr 9, 2023
@andrewrk andrewrk modified the milestones: 0.13.0, 0.12.0 Jun 29, 2023
@jacobly0
Copy link
Member

Related #11812

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants