Skip to content

Zir: implement explicit block_comptime instruction #14819

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

Merged
merged 1 commit into from
Apr 12, 2023

Conversation

mlugg
Copy link
Member

@mlugg mlugg commented Mar 6, 2023

The core goal here was to resolve #7056. I achieved that, but I think this also makes AstGen kinda
easier to understand and less error-prone, which is a nice plus.

Rather than trying to emit comptime-evaluated instructions (e.g. calls marked as comptime, inline
loops), whenever in a comptime scope, AstGen now keeps track of whether Sema will (definitely)
be evaluating the current block at compile-time. When transitioning from runtime code into code
which should be comptime (e.g. a comptime block, a type annotation), AstGen will wrap it in a
block_comptime. As an optimization, this wrapper is elided in a few common cases.

This is a breaking change. There likely exists code in the wild which (intentionally or not) relies
on the dodgy behaviour outlined in #7056, and will need to be changed. Here are a few particular
points:

  • Accessing my_struct.arr.len where my_struct is runtime and has an array field no longer works
    within a comptime block. Whether we want this to work is debatable, but if we do, the bug was
    already present for "real" (i.e. container-level) comptime blocks, so this regression isn't a new
    issue. I did have to change a couple bits of code which relied on this behaviour.

  • comptime return x; in a function called at runtime will now emit a compile error. This seems
    weird at first, but actually makes sense: it's attempted runtime control flow in compile-time
    code! After some discussion on Discord, we concluded that it would probably be good for
    comptime return to implicitly make a function comptime-only. This PR doesn't do that, but it's
    a potential future improvement.

  • Result locations are no longer forwarded across comptime exprs. Instead, the comptime
    expression is evaluated with no result location, and is then retroactively written to the result
    location at maybe-runtime. I'd be impressed if someone found somewhere this matters.

This PR frees up a couple of ZIR tags due to elimination of some instructions which were equivalent
to forced-comptime versions of other instructions. I'll leave it to the core team to decide if they
want to promote some stuff from Extended, or just leave them free for future use.

This PR does increase the size of generated ZIR, but the change isn't significant in most cases. Below
are the total ZIR bytes for a few (somewhat randomly chosen) files from the compiler and standard
library, pre- and post-change. You'll see that hash_map.zig observed by far the biggest change, with
most files sitting around the 1% region.

+------------------------+-----------+-----------+-------------------+
| FILE                   | PRE       | POST      | PERCENTAGE CHANGE |
+------------------------+-----------+-----------+-------------------+
| src/Air.zig            | 73.96 KiB | 74.48 KiB | +0.7%             |
| src/Zir.zig            | 230.7 KiB | 231.8 KiB | +0.5%             |
| src/AstGen,zig         | 1.372 MiB | 1.385 MiB | +1.0%             |
| src/Sema.zig           | 4.308 MiB | 4.334 MiB | +0.6%             |
| lib/std/hash_map.zig   | 252.1 KiB | 265.6 KiB | +5.3%             |
| lib/std/meta.zig       | 138.1 KiB | 141.2 KiB | +2.2%             |
| lib/std/mem.zig        | 529.4 KiB | 541.1 KiB | +2.2%             |
| lib/std/crypto/tls.zig | 38.78 KiB | 39.20 KiB | +1.1%             |
| lib/std/zig/Ast.zig    | 338.8 KiB | 339.8 KiB | +0.2%             |
+------------------------+-----------+-----------+-------------------+

@mlugg mlugg force-pushed the feat/zir-block-comptime branch from fc9d9ae to 2326ef2 Compare March 6, 2023 21:08
@mlugg
Copy link
Member Author

mlugg commented Mar 7, 2023

I think this is actually ready (didn't expect it to pass CI first time!), but I intend to do some basic profiling before marking it ready for review just to make sure I'm not regressing any performance. The CI runtimes look completely normal though, so that's a good sign.

@mlugg mlugg force-pushed the feat/zir-block-comptime branch from 2326ef2 to 16145c2 Compare March 9, 2023 15:44
@mlugg mlugg force-pushed the feat/zir-block-comptime branch from 16145c2 to efee876 Compare April 10, 2023 01:05
@mlugg
Copy link
Member Author

mlugg commented Apr 10, 2023

I tried compiling the self-hosted compiler (in debug mode, with a ReleaseFast compiler) before and after my change, and observed no noteworthy difference in performance (avg ~125s pre, ~124s post). I also tried running the native behaviour tests in ReleaseFast, and again saw no noticeable difference (avg ~15.9s pre, ~16.0s post). Given this and the relatively low change in ZIR size observed, I'm happy to say this doesn't regress perf and mark it ready for review.

(The push after this comment has no changes, it's just changing the commit message, so CI should still pass)

@mlugg mlugg force-pushed the feat/zir-block-comptime branch from efee876 to 5a7328d Compare April 10, 2023 03:25
@mlugg mlugg changed the title WIP: implement explicit block_comptime ZIR instruction Zir: implement explicit block_comptime instruction Apr 10, 2023
@mlugg mlugg marked this pull request as ready for review April 10, 2023 03:25
@andrewrk andrewrk added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Apr 12, 2023
@andrewrk andrewrk merged commit ccf670c into ziglang:master Apr 12, 2023
mlugg added a commit to mlugg/zig that referenced this pull request Apr 15, 2023
motiejus added a commit to motiejus/zig that referenced this pull request May 19, 2023
Since ziglang#14819 this test failed with:

    $ ../../../build/stage3/bin/zig test multi_writer.zig
    multi_writer.zig:26:57: error: unable to evaluate comptime expression
                var batch = std.event.Batch(Error!void, self.streams.len, .auto_async).init();
                                                        ~~~~^~~~~~~~
    referenced by:
        Writer: multi_writer.zig:19:52
        writer: multi_writer.zig:21:36
        remaining reference traces hidden; use '-freference-trace' to see all reference traces

Thanks @jacobly for hints how to fix this on IRC.
andrewrk pushed a commit that referenced this pull request May 19, 2023
Since #14819 this test failed with:

    $ ../../../build/stage3/bin/zig test multi_writer.zig
    multi_writer.zig:26:57: error: unable to evaluate comptime expression
                var batch = std.event.Batch(Error!void, self.streams.len, .auto_async).init();
                                                        ~~~~^~~~~~~~
    referenced by:
        Writer: multi_writer.zig:19:52
        writer: multi_writer.zig:21:36
        remaining reference traces hidden; use '-freference-trace' to see all reference traces

Thanks @jacobly for hints how to fix this on IRC.
@mlugg mlugg deleted the feat/zir-block-comptime branch May 18, 2025 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler allows comptime expression consisting of non-comptime terms
4 participants