run AstGen even when using the stage1 backend

[andrewrk]

In celebration of @g-w1's PR #9047 landing, this changeset reduces the amount of divergence in the compiler's main pipeline logic enough to run AstGen for all files in the compilation, regardless of whether the stage1 or stage2 backend is being used.

Practically, this means that all Zig code is subject to new compile errors, such as unused local variables.

Implements #8970

Additionally:

• remove leftover unsound asserts from recent hash map changes
• fix sub-Compilation errors not indenting correctly
• CI: no longer pass --ast-check to zig fmt because that is now redundant.

Perf

This is doing strictly more work, so I expect perf to be strictly worse. This is testing on my beefy dell laptop. One observation I make here is that the perf hit with a warm cache is negligible, and the cache system is granular enough that the benefits apply to practically every compilation. Another observation is that even with a cold cache, for large compilations the perf hit is negligible.

Hello World, cold cache

• wall clock: 1.40s to 1.67s (20% slower)
• peak rss: 254 MiB to 338 MiB (33% more)

Hello World, warm cache

• wall clock: 0s (no change)
• peak rss: 42 Mib to 96 Mib (127% more)

std lib tests, cold cache

• wall clock: 41s (no change)
• peak rss: 8.4 GiB to 8.5 GiB (1% more)

std lib tests, warm cache

• wall clock: 0s (no change)
• peak rss: 122 MiB (no change)

Merge blockers:

• Figure out why this is happening:

error: unable to load std: FileNotFound
error: unable to load test/std: FileNotFound
error: unable to load test/src/std: FileNotFound

It was happening because of #9204.

• translate-c generates zig code with unnecessary _ = foo; statements for unused local variables and parameters #9205
• AstGen crash when struct field missing type #9206
• get the translate-c tests passing
• update the language reference docs

Related issues:

• prevent redundant AstGen compile errors #9192
• compile error "local shadows declaration" only detects previous decls #9194
• improve error message when chasing AstGen imports recursively and file does not exist #9203
• self-hosted compiler does not map 'std' into every package #9204

[andrewrk]

This PR breaks godbolt again, and we should be sure to shoot them a friendly PR this time: b9d86a1

[andrewrk]

Alright, this PR is now blocking on translate-c generating zig code with unused local variables. @ehaas I don't suppose you're interested in tackling this with me?

[ehaas]

Ok, the latest translate-c problem is this:

C code

#include <stdint.h>
typedef int16_t  __v4hi __attribute__((__vector_size__(8)));
int main(void) {
    __v4hi v4_a = {0, 0, 0, 0};
    __v4hi v4_b = {0, 0, 0, 0};
    __v4hi shuffled = __builtin_shufflevector(v4_a, v4_b, 0, 0, 0, 0);
    return 0;
}

Cleaned up but equivalent translated Zig looks like this:

const std = @import("std");
const __v4hi = std.meta.Vector(4, i16);
pub fn main() void {
    var v4_a = __v4hi {0, 0, 0, 0};
    var v4_b = __v4hi {0, 0, 0, 0};
    var shuffled: __v4hi = @shuffle(i16, v4_a, v4_b, comptime std.meta.Vector(4, i32){
        std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
        std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
        std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
        std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
    });
    _ = shuffled;
}

The problem is that comptime on the shuffle mask. With it, we get error: redundant comptime keyword in already comptime scope. Without it, we get error: unable to evaluate constant expression

Moving the mask into its own variable const shufflemask = comptime std.meta.Vector(4, i32){... prevents the error but I wanted to check if that is the proper solution, or is this actually revealing some other bug.

[SpexGuy]

This feels like a false positive to me for the redundant comptime keyword, but a workaround is to put parentheses around the whole expression, including the comptime. So

    var shuffled: __v4hi = @shuffle(i16, v4_a, v4_b, (comptime std.meta.Vector(4, i32){
        std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
        std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
        std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
        std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
    }));

Or you could put comptime on each internall call, like

    var shuffled: __v4hi = @shuffle(i16, v4_a, v4_b, std.meta.Vector(4, i32){
        comptime std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
        comptime std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
        comptime std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
        comptime std.zig.c_translation.shuffleVectorIndex(0, @typeInfo(@TypeOf(v4_a)).Vector.len),
    });

[g-w1]

The reason it is like this is because shuffle is defined in astgen like this:

        .shuffle => {
            const result = try gz.addPlNode(.shuffle, node, Zir.Inst.Shuffle{
                .elem_type = try typeExpr(gz, scope, params[0]),
                .a = try expr(gz, scope, .none, params[1]),
                .b = try expr(gz, scope, .none, params[2]),
                .mask = try comptimeExpr(gz, scope, .none, params[3]),
            });
            return rvalue(gz, rl, result, node);
        },

Note how mask is comptimeExpr. I think this workaround will not be needed when stage2 is switched to fully.

[ehaas]

I'm still getting the error even with parentheses around the whole expression, and with the internal comptime calls. I think extracting it into its own variable won't be terribly difficult.

[andrewrk]

It's not a false positive. The mask parameter of @shuffle is comptime, already forcing the entire expression to be comptime. However, stage1 does not implement this correctly. A quick bug fix to stage1 will solve the problem. Give me a few min

[andrewrk]

Fixed in fc185a6

[ehaas]

How do you want to manage the commit history on this one? I could cherry-pick that change into my branch (along with the just-commited extern enum removal) and meld it all together, or I could create a PR against master that removes comptime from translate-c's shuffle mask expression.

[andrewrk]

@ehaas I merged your PR and @g-w1's PRs into this branch, rebased against master to grab the stage1 fix. So I think the next step would be opening a new PR against this branch. Sound OK?

[ehaas]

It looks like the extern enum stuff didn't make it in from the rebase? I think that's why it's saying there's a conflict.

[andrewrk]

What extern enum stuff?

[ehaas]

#9164 got merged to master but it looks like it's not in stage1-astcheck

[andrewrk]

I don't know what went wrong with the rebase before, I must have made some kind of mistake. Anyway it looks like the only remaining item left is to update the language reference (which I'm working on). Thanks @ehaas and @g-w1!

[andrewrk]

Oops, spoke too soon. Looks like this test is failing:

#include <stdlib.h>
int main(void) {
   enum Foo { A, B, C };
   enum Foo a = B;
   if (a != B) abort();
   if (a != 1) abort();
   {
      enum Foo { A = 5, B = 6, C = 7 };
      enum Foo a = B;
      if (a != B) abort();
      if (a != 6) abort();
   }
   if (a != B) abort();
   if (a != 1) abort();
}

generates

pub export fn main() c_int {
    const A: c_int = 0;
    const B: c_int = 1;
    const C: c_int = 2;
    const enum_Foo = c_uint;
    var a: enum_Foo = @bitCast(c_uint, B);
    _ = a;
    if (a != @bitCast(c_uint, B)) {
        abort();
    }
    if (a != @bitCast(c_uint, @as(c_int, 1))) {
        abort();
    }
    {
        const A_2: c_int = 5;
        const B_3: c_int = 6;
        const C_4: c_int = 7;
        const enum_Foo_1 = c_uint;
        var a_5: enum_Foo_1 = @bitCast(c_uint, B_3);
        _ = a_5;
        if (a_5 != @bitCast(c_uint, B_3)) {
            abort();
        }
        if (a_5 != @bitCast(c_uint, @as(c_int, 6))) {
            abort();
        }
    }
    if (a != @bitCast(c_uint, B)) {
        abort();
    }
    if (a != @bitCast(c_uint, @as(c_int, 1))) {
        abort();
    }
    return 0;
}

source.zig:458:15: error: unused local constant
        const C_4: c_int = 7;
              ^

[ehaas]

Ok, looks like I need to generate discards for non-top-level enum constants. Should be straightforward. Can you push the updated rebase

[Vexu]

The issue with discards could also be solved by partially implementing #2984 and disabling the unused variable check if the keyword is present.