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

Sema: rewrite peer type resolution #15726

Merged
merged 3 commits into from
Jun 14, 2023

Conversation

mlugg
Copy link
Member

@mlugg mlugg commented May 16, 2023

The existing logic for peer type resolution was quite convoluted and buggy. This rewrite makes it much more resilient, readable, and extensible. The algorithm works by first iterating over the types to select a "strategy", then applying that strategy, possibly applying peer resolution recursively.

Several new tests have been added to cover cases which the old logic did not correctly handle.

Resolves: #15138
Resolves: #15644
Resolves: #15693
Resolves: #15709
Resolves: #15752

@andrewrk andrewrk self-requested a review May 16, 2023 05:33
@mlugg mlugg force-pushed the feat/peer-type-resolution-but-better branch 4 times, most recently from 72c7c3e to 05c8d36 Compare May 16, 2023 16:34
@mlugg
Copy link
Member Author

mlugg commented May 16, 2023

I ran each of the following tests 5 times on my system (Ryzen 7 1800x @ 3.6 GHz; 16GB DDR4 @ 2133 MT/s; Void Linux).
The system was under minimal other load. All tests were run with a clean cache.

Test Old New % change
Build behaviour tests 11.5s 11.2s -3%
Build compiler (only-c) 26.1s 25.4s -3% (note: high variability)
Build ptr_stress.zig 4.45s 4.54s +2%

ptr_stress.zig is a little test program I wrote with the intention of running PTR with a large number of peers.

ptr_stress.zig
//! This is a nonsense program meant to hit Peer Type Resolution code in expensive ways.
//! Intended for benchmarking the compiler on PTR-heavy code.

const std = @import("std");

fn ptrThing(comptime seed: u32) void {
    var x: u10 = undefined;

    @setEvalBranchQuota(100_000);

    const res = switch (x) {
        // We need these because old PTR is order-dependent
        // (and also crashes if we include an error set below)
        0 => @as(u64, 0),
        1 => error.Foo,
        inline else => |x_ct| blk: {
            const ResultType: type = switch ((seed *% @as(u32, x_ct)) % 6) {
                0 => @TypeOf(null),
                1 => ?u64,
                2 => u64,
                3 => u32,
                4 => noreturn,
                5 => @TypeOf(undefined),
                else => unreachable,
            };

            if (ResultType == noreturn) {
                break :blk while (true) {};
            } else {
                break :blk @as(ResultType, undefined);
            }
        },
    };

    if (@TypeOf(res) != error{Foo}!?u64) {
        @compileError(std.fmt.comptimePrint("Unexpected result type: {}", .{@TypeOf(res)}));
    }
}

pub fn main() void {
    ptrThing(747079);
    ptrThing(723898);
    ptrThing(69178);
    ptrThing(202813);
    ptrThing(110022);
}

Of course, this change won't actually improve performance for anything - you could get more accurate
numbers here by profiling under more controlled conditions and doing far more repeats (manually doing
them becomes tedious quickly). But it seems that the impact this change has on performance is negligible.

@mlugg mlugg force-pushed the feat/peer-type-resolution-but-better branch 4 times, most recently from 139bd01 to 4f8e384 Compare May 17, 2023 08:02
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.

Thank you for doing this work, and for providing perf measurements.

I have requested changes based on the behavior changes alone; I have not looked at Sema.zig yet.

Also please don't mark this as closing #9917 since there is a translate-c bug in that issue as well as a peer type resolution bug.

test/behavior/cast.zig Outdated Show resolved Hide resolved
lib/std/crypto/kyber_d00.zig Outdated Show resolved Hide resolved
lib/std/fmt/parse_float/parse.zig Outdated Show resolved Hide resolved
lib/std/macho.zig Outdated Show resolved Hide resolved
@mlugg mlugg force-pushed the feat/peer-type-resolution-but-better branch 4 times, most recently from 7ebaf08 to cfdf532 Compare May 20, 2023 21:47
@mlugg mlugg requested a review from andrewrk May 20, 2023 21:48
@mlugg mlugg force-pushed the feat/peer-type-resolution-but-better branch 3 times, most recently from ec965b9 to 95a937b Compare May 21, 2023 22:38
@mlugg mlugg force-pushed the feat/peer-type-resolution-but-better branch from 95a937b to 1b6f3f5 Compare June 13, 2023 14:38
@andrewrk andrewrk enabled auto-merge June 13, 2023 20:46
The existing logic for peer type resolution was quite convoluted and
buggy. This rewrite makes it much more resilient, readable, and
extensible. The algorithm works by first iterating over the types to
select a "strategy", then applying that strategy, possibly applying peer
resolution recursively.

Several new tests have been added to cover cases which the old logic did
not correctly handle.

Resolves: ziglang#15138
Resolves: ziglang#15644
Resolves: ziglang#15693
Resolves: ziglang#15709
Resolves: ziglang#15752
This allows tuples whose fields are in-memory coercible to themselves be
coerced in memory. No InMemoryCoercionResult field has been added, so in
future one could be added to improve error messages.
Previously, this logic was split between Sema.coerceValueInMemory and
InternPool.getCoerced. This led to issues when trying to coerce e.g. an
optional containing an aggregate, because we'd call through to
InternPool's version which only recurses on itself so could not coerce
aggregates. Unifying them is fairly simple, and also simplified a bit of
logic in Sema.

Also fixes a key lifetime bug in aggregate coercion.
auto-merge was automatically disabled June 13, 2023 20:48

Head branch was pushed to by a user without write access

@mlugg mlugg force-pushed the feat/peer-type-resolution-but-better branch from 9a80fb7 to 8a92beb Compare June 13, 2023 20:48
@andrewrk andrewrk enabled auto-merge June 13, 2023 20:49
@andrewrk andrewrk merged commit 496320d into ziglang:master Jun 14, 2023
@dweiller dweiller mentioned this pull request Aug 19, 2023
TUSF pushed a commit to TUSF/zig that referenced this pull request May 9, 2024
…-but-better

Sema: rewrite peer type resolution
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants