Sema: rewrite semantic analysis of function calls #22414

mlugg · 2025-01-05T05:40:03Z

This rewrite improves some error messages, hugely simplifies the logic, and fixes several bugs. One of these bugs is technically a new rule which Andrew and I agreed on: if a parameter has a comptime-only type but is not declared comptime, then the corresponding call argument should not be evaluated at comptime; only resolved. Implementing this required changing how function types work a little, which in turn required allowing a new kind of function coercion for some generic use cases: function coercions are now allowed to implicitly remove comptime annotations from parameters with comptime-only types. This is okay because removing the annotation affects only the call site.

Resolves: #22262

This is a breaking change, because it fixes a bug relating to eval branch quota. Previously, the usage of a "child Sema" when analyzing a generic function's call site meant we accidentally "duplicated" the eval branch quota. We no longer do this (I've actually concluded that the concept of a "child Sema" isn't really coherent), so a little more eval branch quota is used up on evaluating generic parameter/return types. That's why the changes to lib/compiler/aro_translate_c.zig and src/translate_c.zig were necessary. #22410 is not resolved by this PR; the buggy behavior of status quo is left intact.

I was considering also reworking the ZIR for function declarations in this PR, since it's very overcomplicated. However, I decided to leave that for now, since I'm planning to start playing around with a biiiig ZIR rewrite soon.

andrewrk

please provide a perf data point
please note the before/after compiler recursion stack limit (e.g. how much comptime recursive fibonacci can you do before crashing the compiler)

src/Sema.zig

mlugg · 2025-01-09T05:12:53Z

Performance Data Points

Analyze `std` Tests

Benchmark 1 (6 runs): /home/mlugg/zig/master/stage4-fast/bin/zig test --zig-lib-dir lib lib/std/std.zig -fno-emit-bin
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          10.1s  ± 12.3ms    10.1s  … 10.1s           0 ( 0%)        0%
  peak_rss            595MB ±  170KB     595MB …  595MB          0 ( 0%)        0%
  cpu_cycles         43.2G  ± 39.5M     43.2G  … 43.3G           0 ( 0%)        0%
  instructions       84.6G  ± 37.2K     84.6G  … 84.6G           0 ( 0%)        0%
  cache_references   3.21G  ± 8.74M     3.21G  … 3.23G           0 ( 0%)        0%
  cache_misses       80.4M  ± 2.08M     78.1M  … 84.0M           0 ( 0%)        0%
  branch_misses      63.8M  ±  192K     63.5M  … 64.0M           0 ( 0%)        0%
Benchmark 2 (7 runs): /home/mlugg/zig/better-analyze-call/stage4-fast/bin/zig test --zig-lib-dir lib lib/std/std.zig -fno-emit-bin
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          9.97s  ± 20.4ms    9.94s  … 9.99s           0 ( 0%)        ⚡-  1.6% ±  0.2%
  peak_rss            594MB ±  201KB     594MB …  594MB          0 ( 0%)          -  0.2% ±  0.0%
  cpu_cycles         42.5G  ± 90.4M     42.4G  … 42.7G           0 ( 0%)        ⚡-  1.6% ±  0.2%
  instructions       83.5G  ± 21.5K     83.5G  … 83.5G           0 ( 0%)        ⚡-  1.3% ±  0.0%
  cache_references   3.10G  ± 8.62M     3.09G  … 3.12G           0 ( 0%)        ⚡-  3.6% ±  0.3%
  cache_misses       84.0M  ± 2.06M     81.3M  … 86.4M           0 ( 0%)        💩+  4.5% ±  3.2%
  branch_misses      63.9M  ±  645K     63.0M  … 64.9M           0 ( 0%)          +  0.2% ±  0.9%

Analyze Compiler

Benchmark 1 (8 runs): /home/mlugg/zig/master/stage4-fast/bin/zig build-exe --dep aro --dep aro_translate_c --dep build_options -Mroot=src/main.zig -Maro=lib/compiler/aro/aro.zig --dep aro -Maro_translate_c=lib/compiler/aro_translate_c.zig -Mbuild_options=.zig-cache/c/9883e0613966ebf6eeefe29d780bb845/options.zig -fno-emit-bin --zig-lib-dir lib
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          7.58s  ± 56.5ms    7.53s  … 7.72s           2 (25%)        0%
  peak_rss            389MB ±  150KB     389MB …  389MB          0 ( 0%)        0%
  cpu_cycles         32.5G  ±  222M     32.3G  … 33.0G           1 (13%)        0%
  instructions       58.8G  ± 22.1K     58.8G  … 58.8G           0 ( 0%)        0%
  cache_references   2.78G  ± 9.54M     2.76G  … 2.79G           0 ( 0%)        0%
  cache_misses        126M  ± 1.12M      124M  …  127M           0 ( 0%)        0%
  branch_misses      89.8M  ±  546K     89.3M  … 91.0M           1 (13%)        0%
Benchmark 2 (9 runs): /home/mlugg/zig/better-analyze-call/stage4-fast/bin/zig build-exe --dep aro --dep aro_translate_c --dep build_options -Mroot=src/main.zig -Maro=lib/compiler/aro/aro.zig --dep aro -Maro_translate_c=lib/compiler/aro_translate_c.zig -Mbuild_options=.zig-cache/c/9883e0613966ebf6eeefe29d780bb845/options.zig -fno-emit-bin --zig-lib-dir lib
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          7.45s  ± 50.8ms    7.40s  … 7.54s           0 ( 0%)        ⚡-  1.8% ±  0.7%
  peak_rss            384MB ±  193KB     384MB …  384MB          0 ( 0%)        ⚡-  1.3% ±  0.0%
  cpu_cycles         31.9G  ±  210M     31.7G  … 32.3G           0 ( 0%)        ⚡-  1.7% ±  0.7%
  instructions       57.5G  ± 24.4K     57.5G  … 57.5G           0 ( 0%)        ⚡-  2.3% ±  0.0%
  cache_references   2.66G  ± 11.3M     2.65G  … 2.69G           0 ( 0%)        ⚡-  4.1% ±  0.4%
  cache_misses        127M  ± 2.31M      124M  …  131M           0 ( 0%)          +  1.2% ±  1.5%
  branch_misses      88.5M  ±  552K     87.9M  … 89.5M           0 ( 0%)          -  1.4% ±  0.6%

TL;DR: faster 😎

Maximum Comptime Recursion Depth

Running zig build-obj foo.zig on this file:

comptime {
    @setEvalBranchQuota(10_000);
    @compileLog(fib(1234)); // replacing this value
}

fn fib(n: comptime_int) comptime_int {
    if (n == 0) return 1;
    return fib(n - 1) * n;
}

master: 4062
This branch: 2854 (-30%)

TL;DR: worse 😢

@andrewrk, if this is an issue, I can probably improve it by splitting out the argument-evaluating logic (which has a Block and stuff) into a separate function to decrease the size of the main inline call stack frame.

This rewrite improves some error messages, hugely simplifies the logic, and fixes several bugs. One of these bugs is technically a new rule which Andrew and I agreed on: if a parameter has a comptime-only type but is not declared `comptime`, then the corresponding call argument should not be *evaluated* at comptime; only resolved. Implementing this required changing how function types work a little, which in turn required allowing a new kind of function coercion for some generic use cases: function coercions are now allowed to implicitly *remove* `comptime` annotations from parameters with comptime-only types. This is okay because removing the annotation affects only the call site. Resolves: ziglang#22262

Before the prior commit, the maximum comptime recursion depth on my system was 4062. After the prior commit, it decreased to 2854. This commit increases the compiler's stack size enough so that the recursion depth limit is no less than it was before the `Sema.analyzeCall` rewrite, preventing this from being a breaking change. Specifically, this stack size increases my observed maximum comptime recursion depth to 4105.

dnjulek · 2025-01-11T14:52:04Z

It seems that this has broken ZLS build

zls git:(master) zig build -Doptimize=ReleaseSafe
install
└─ install zls
   └─ zig build-exe zls ReleaseSafe native 1 errors
/usr/local/lib/zig/std/mem.zig:669:5: error: evaluation exceeded 1000 backwards branches
    for (a, b) |a_elem, b_elem| {
    ^~~
/usr/local/lib/zig/std/mem.zig:669:5: note: use @setEvalBranchQuota() to raise the branch limit from 1000
.zig-cache/o/62617b606a626867bfe5e239ceb48dc2/lsp_types.zig:41:24: note: called from here
        if (std.mem.eql(u8, method, meta.method)) {
            ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~

zig: 0.14.0-dev.2634+b36ea592b
zls: a9e651a2456893e41c2a72af354a226a953a8253

mlugg · 2025-01-11T15:36:12Z

ZLS is unaffiliated with the Zig project -- please open an issue on the ZLS repo instead.

The original motivation here was to fix regressions caused by ziglang#22414. However, while working on this, I ended up discussing a language simplification with Andrew, which changes things a little from how they worked before ziglang#22414. The main user-facing change here is that any reference to a prior function parameter, even if potentially comptime-known at the usage site or even not analyzed, now makes a function generic. This applies even if the parameter being referenced is not a `comptime` parameter, since it could still be populated when performing an inline call. This is a breaking language change. The detection of this is done in AstGen; when evaluating a parameter type or return type, we track whether it referenced any prior parameter, and if so, we mark this type as being "generic" in ZIR. This will cause Sema to not evaluate it until the time of instantiation or inline call. A lovely consequence of this from an implementation perspective is that it eliminates the need for most of the "generic poison" system. In particular, `error.GenericPoison` is now completely unnecessary, because we identify generic expressions earlier in the pipeline; this simplifies the compiler and avoids redundant work. This also entirely eliminates the concept of the "generic poison value". The only remnant of this system is the "generic poison type" (`Type.generic_poison` and `InternPool.Index.generic_poison_type`). This type is used in two places: * During semantic analysis, to represent an unknown result type. * When storing generic function types, to represent a generic parameter/return type. It's possible that these use cases should instead use `.none`, but I leave that investigation to a future adventurer. One last thing. Prior to ziglang#22414, inline calls were a little inefficient, because they re-evaluated even non-generic parameter types whenever they were called. Changing this behavior is what ultimately led to ziglang#22538. Well, because the new logic will mark a type expression as generic if there is any change its resolved type could differ in an inline call, this redundant work is unnecessary! So, this is another way in which the new design reduces redundant work and complexity. Resolves: ziglang#22494 Resolves: ziglang#22532 Resolves: ziglang#22538

The original motivation here was to fix regressions caused by #22414. However, while working on this, I ended up discussing a language simplification with Andrew, which changes things a little from how they worked before #22414. The main user-facing change here is that any reference to a prior function parameter, even if potentially comptime-known at the usage site or even not analyzed, now makes a function generic. This applies even if the parameter being referenced is not a `comptime` parameter, since it could still be populated when performing an inline call. This is a breaking language change. The detection of this is done in AstGen; when evaluating a parameter type or return type, we track whether it referenced any prior parameter, and if so, we mark this type as being "generic" in ZIR. This will cause Sema to not evaluate it until the time of instantiation or inline call. A lovely consequence of this from an implementation perspective is that it eliminates the need for most of the "generic poison" system. In particular, `error.GenericPoison` is now completely unnecessary, because we identify generic expressions earlier in the pipeline; this simplifies the compiler and avoids redundant work. This also entirely eliminates the concept of the "generic poison value". The only remnant of this system is the "generic poison type" (`Type.generic_poison` and `InternPool.Index.generic_poison_type`). This type is used in two places: * During semantic analysis, to represent an unknown result type. * When storing generic function types, to represent a generic parameter/return type. It's possible that these use cases should instead use `.none`, but I leave that investigation to a future adventurer. One last thing. Prior to #22414, inline calls were a little inefficient, because they re-evaluated even non-generic parameter types whenever they were called. Changing this behavior is what ultimately led to #22538. Well, because the new logic will mark a type expression as generic if there is any change its resolved type could differ in an inline call, this redundant work is unnecessary! So, this is another way in which the new design reduces redundant work and complexity. Resolves: #22494 Resolves: #22532 Resolves: #22538

The original motivation here was to fix regressions caused by ziglang#22414. However, while working on this, I ended up discussing a language simplification with Andrew, which changes things a little from how they worked before ziglang#22414. The main user-facing change here is that any reference to a prior function parameter, even if potentially comptime-known at the usage site or even not analyzed, now makes a function generic. This applies even if the parameter being referenced is not a `comptime` parameter, since it could still be populated when performing an inline call. This is a breaking language change. The detection of this is done in AstGen; when evaluating a parameter type or return type, we track whether it referenced any prior parameter, and if so, we mark this type as being "generic" in ZIR. This will cause Sema to not evaluate it until the time of instantiation or inline call. A lovely consequence of this from an implementation perspective is that it eliminates the need for most of the "generic poison" system. In particular, `error.GenericPoison` is now completely unnecessary, because we identify generic expressions earlier in the pipeline; this simplifies the compiler and avoids redundant work. This also entirely eliminates the concept of the "generic poison value". The only remnant of this system is the "generic poison type" (`Type.generic_poison` and `InternPool.Index.generic_poison_type`). This type is used in two places: * During semantic analysis, to represent an unknown result type. * When storing generic function types, to represent a generic parameter/return type. It's possible that these use cases should instead use `.none`, but I leave that investigation to a future adventurer. One last thing. Prior to ziglang#22414, inline calls were a little inefficient, because they re-evaluated even non-generic parameter types whenever they were called. Changing this behavior is what ultimately led to ziglang#22538. Well, because the new logic will mark a type expression as generic if there is any change its resolved type could differ in an inline call, this redundant work is unnecessary! So, this is another way in which the new design reduces redundant work and complexity. Resolves: ziglang#22494 Resolves: ziglang#22532 Resolves: ziglang#22538

mlugg added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Jan 5, 2025

mlugg requested a review from andrewrk January 5, 2025 05:48

mlugg force-pushed the better-analyze-call branch from 456f5be to 9acff76 Compare January 5, 2025 06:12

andrewrk added the release notes This PR should be mentioned in the release notes. label Jan 5, 2025

mlugg force-pushed the better-analyze-call branch 2 times, most recently from 5b130ec to 383b8ff Compare January 5, 2025 11:40

mlugg mentioned this pull request Jan 5, 2025

compiler: improve "... contains reference to comptime var" errors #22425

Merged

mlugg force-pushed the better-analyze-call branch from 383b8ff to d5c1789 Compare January 6, 2025 01:54

andrewrk requested changes Jan 8, 2025

View reviewed changes

src/Sema.zig Show resolved Hide resolved

src/Sema.zig Show resolved Hide resolved

mlugg force-pushed the better-analyze-call branch from d5c1789 to e9bd2d4 Compare January 9, 2025 06:46

mlugg force-pushed the better-analyze-call branch from c6f4bbc to 6a837e6 Compare January 10, 2025 05:41

mlugg enabled auto-merge January 10, 2025 05:44

mlugg merged commit bc846c3 into ziglang:master Jan 10, 2025
10 checks passed

alexrp mentioned this pull request Jan 15, 2025

Comptime known value not comptime known in SDL3.h translate-c #22494

Closed

This was referenced Jan 18, 2025

translate-c regression: type error on | of c_uint and c_int #22532

Closed

argument of inline fn no longer propagates comptime-ness to result type expression #22538

Closed

mlugg mentioned this pull request Jan 19, 2025

compiler: simplify generic functions, fix issues with inline calls #22547

Merged

mlugg deleted the better-analyze-call branch May 18, 2025 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Sema: rewrite semantic analysis of function calls #22414

Sema: rewrite semantic analysis of function calls #22414

Uh oh!

mlugg commented Jan 5, 2025 •

edited

Loading

Uh oh!

andrewrk left a comment

Uh oh!

Uh oh!

Uh oh!

mlugg commented Jan 9, 2025 •

edited

Loading

Uh oh!

Uh oh!

dnjulek commented Jan 11, 2025

Uh oh!

mlugg commented Jan 11, 2025

Uh oh!

Uh oh!

Uh oh!

Sema: rewrite semantic analysis of function calls #22414

Sema: rewrite semantic analysis of function calls #22414

Uh oh!

Conversation

mlugg commented Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mlugg commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance Data Points

Analyze std Tests

Analyze Compiler

Maximum Comptime Recursion Depth

Uh oh!

Uh oh!

dnjulek commented Jan 11, 2025

Uh oh!

mlugg commented Jan 11, 2025

Uh oh!

Uh oh!

mlugg commented Jan 5, 2025 •

edited

Loading

mlugg commented Jan 9, 2025 •

edited

Loading

Analyze `std` Tests