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

Stage2: implement comptime variables #9030

Merged
merged 2 commits into from
Jun 8, 2021
Merged

Stage2: implement comptime variables #9030

merged 2 commits into from
Jun 8, 2021

Conversation

Vexu
Copy link
Member

@Vexu Vexu commented Jun 7, 2021

Not sure how well this will work with arrays and structs but the invalid store logic should be good.

Inline loops do not work yet due to some other bug in analysis.

Implements #1470 but not #7396.

@andrewrk
Copy link
Member

andrewrk commented Jun 7, 2021

I just played with this a little bit, and it looks great! It's nice to see this being massively simpler with the SSA in tree form than how it is in stage1.

Here's another set of passing test cases you can add:

comptime {
    var x: i32 = 1;
    x += 1;
    if (x != 2) unreachable;
}
comptime {
    var x: i32 = 1;
    x += 1;
    if (x != 1) unreachable;
}

The second one correctly gives:

test.zig:4:17: error: unable to resolve comptime value
    if (x != 1) unreachable;
                ^

Regarding the todo about source location, I'll have a look at that and see if I can help.

@andrewrk
Copy link
Member

andrewrk commented Jun 7, 2021

Here's the solution to the source location issue:

diff --git a/src/Sema.zig b/src/Sema.zig
index 0dd9ea47e..ea5a0e377 100644
--- a/src/Sema.zig
+++ b/src/Sema.zig
@@ -4607,14 +4607,14 @@ fn zirArithmetic(sema: *Sema, block: *Scope.Block, inst: Zir.Inst.Index) InnerEr
 
     const tag_override = block.sema.code.instructions.items(.tag)[inst];
     const inst_data = sema.code.instructions.items(.data)[inst].pl_node;
-    const src: LazySrcLoc = .{ .node_offset_bin_op = inst_data.src_node };
+    sema.src = .{ .node_offset_bin_op = inst_data.src_node };
     const lhs_src: LazySrcLoc = .{ .node_offset_bin_lhs = inst_data.src_node };
     const rhs_src: LazySrcLoc = .{ .node_offset_bin_rhs = inst_data.src_node };
     const extra = sema.code.extraData(Zir.Inst.Bin, inst_data.payload_index).data;
     const lhs = try sema.resolveInst(extra.lhs);
     const rhs = try sema.resolveInst(extra.rhs);
 
-    return sema.analyzeArithmetic(block, tag_override, lhs, rhs, src, lhs_src, rhs_src);
+    return sema.analyzeArithmetic(block, tag_override, lhs, rhs, sema.src, lhs_src, rhs_src);
 }
 
 fn zirOverflowArithmetic(

Generally, the idea here is to set sema.src for any instruction which has a source location, allowing AstGen to skip supplying the source location if it knows it would be redundant with the previous instruction.

@Vexu
Copy link
Member Author

Vexu commented Jun 7, 2021

Here's the solution to the source location issue:

Thanks, added.

Also to clarify on the inline loop issue, I checked that the loads and stores are happening correctly but for some reason the condition is only evaluated once. You'll probably be able to find the issue much quicker than me since you wrote all of the inlining logic.

comptime {
    var i: u64 = 0;
    inline while (i < 5) : (i+=5) {} // :3:12: error: evaluation exceeded 1000 backwards branches
}

@andrewrk
Copy link
Member

andrewrk commented Jun 7, 2021

The problem was repeat_inline tried to set i = 0; but then the continue expression (i += 1) was making it skip over the condition. Solution:

diff --git a/src/Sema.zig b/src/Sema.zig
index 0dd9ea47e..6145b8c13 100644
--- a/src/Sema.zig
+++ b/src/Sema.zig
@@ -147,7 +147,7 @@ pub fn analyzeBody(
     // directly jump to the next one, rather than detouring through the loop
     // continue expression. Related: https://github.com/ziglang/zig/issues/8220
     var i: usize = 0;
-    while (true) : (i += 1) {
+    while (true) {
         const inst = body[i];
         const air_inst = switch (tags[inst]) {
             // zig fmt: off
@@ -394,78 +394,97 @@ pub fn analyzeBody(
             // putting them into the map.
             .breakpoint => {
                 try sema.zirBreakpoint(block, inst);
+                i += 1;
                 continue;
             },
             .fence => {
                 try sema.zirFence(block, inst);
+                i += 1;
                 continue;
             },
             .dbg_stmt => {
                 try sema.zirDbgStmt(block, inst);
+                i += 1;
                 continue;
             },
             .ensure_err_payload_void => {
                 try sema.zirEnsureErrPayloadVoid(block, inst);
+                i += 1;
                 continue;
             },
             .ensure_result_non_error => {
                 try sema.zirEnsureResultNonError(block, inst);
+                i += 1;
                 continue;
             },
             .ensure_result_used => {
                 try sema.zirEnsureResultUsed(block, inst);
+                i += 1;
                 continue;
             },
             .set_eval_branch_quota => {
                 try sema.zirSetEvalBranchQuota(block, inst);
+                i += 1;
                 continue;
             },
             .store => {
                 try sema.zirStore(block, inst);
+                i += 1;
                 continue;
             },
             .store_node => {
                 try sema.zirStoreNode(block, inst);
+                i += 1;
                 continue;
             },
             .store_to_block_ptr => {
                 try sema.zirStoreToBlockPtr(block, inst);
+                i += 1;
                 continue;
             },
             .store_to_inferred_ptr => {
                 try sema.zirStoreToInferredPtr(block, inst);
+                i += 1;
                 continue;
             },
             .resolve_inferred_alloc => {
                 try sema.zirResolveInferredAlloc(block, inst);
+                i += 1;
                 continue;
             },
             .validate_struct_init_ptr => {
                 try sema.zirValidateStructInitPtr(block, inst);
+                i += 1;
                 continue;
             },
             .validate_array_init_ptr => {
                 try sema.zirValidateArrayInitPtr(block, inst);
+                i += 1;
                 continue;
             },
             .@"export" => {
                 try sema.zirExport(block, inst);
+                i += 1;
                 continue;
             },
             .set_align_stack => {
                 try sema.zirSetAlignStack(block, inst);
+                i += 1;
                 continue;
             },
             .set_cold => {
                 try sema.zirSetAlignStack(block, inst);
+                i += 1;
                 continue;
             },
             .set_float_mode => {
                 try sema.zirSetFloatMode(block, inst);
+                i += 1;
                 continue;
             },
             .set_runtime_safety => {
                 try sema.zirSetRuntimeSafety(block, inst);
+                i += 1;
                 continue;
             },
 
@@ -510,6 +529,7 @@ pub fn analyzeBody(
         if (air_inst.ty.isNoReturn())
             return always_noreturn;
         try map.put(sema.gpa, inst, air_inst);
+        i += 1;
     }
 }
 

Confirmed this fixes the test case you posted above, as well as existing stage2 tests.

@andrewrk
Copy link
Member

andrewrk commented Jun 8, 2021

The failure is:

Expected error not found:
================================================================================
:4:17: error: unable to resolve comptime value
================================================================================
update_index=3 test 'comptime var' failed: WrongCompileErrors

The test case is exhibiting correct behavior:

pub fn main() void {
    comptime var x: i32 = 1;
    x += 1;
    if (x != 1) unreachable;
}

It correctly compiles OK, and then traps when it hits unreachable at runtime.

The test case should be changed to:

comptime {
    var x: i32 = 1;
    x += 1;
    if (x != 1) unreachable; // error: unable to resolve comptime value
}

pub fn main() void {}

Co-authored-by: Andrew Kelley <andrew@ziglang.org>
@andrewrk andrewrk merged commit ccfa168 into ziglang:master Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants