Commit 8509e7111d80a07e778aa2a57d58d2bea6945014 | zig

author	Andrew Kelley <andrew@ziglang.org> 2021-10-25 15:06:47-07:00
committer	Andrew Kelley <andrew@ziglang.org> 2021-10-25 15:11:21-07:00
log	8509e7111d80a07e778aa2a57d58d2bea6945014
tree	e7eedaf980ada713a6c3d2da3e6153751d03bed6
parent	a132190cad80669306705b72276e9641401426fb

stage2: fix switch on tagged union capture-by-pointer

 * AstGen: always use `typeof` and never `typeof_elem` on the
   `switch_cond`/`switch_cond_ref` instruction because both variants
   return a value and not a pointer.
   - Delete the `typeof_elem` ZIR instruction since it is no longer
     needed.
 * Sema: validateUnionInit now recognizes a comptime mutable value and
   no longer emits a compile error saying "cannot evaluate constant
   expression"
   - Still to-do is detecting comptime union values in a function that
     is not being executed at compile-time.
     - This is still to-do for structs too.
 * Sema: when emitting a call AIR instruction, call resolveTypeLayout on
   all the parameter types as well as the return type.
 * `Type.structFieldOffset` now works for unions in addition to structs.

7 files changed, 115 insertions(+), 97 deletions(-)

src
test
- behavior
  - switch.zig
  - switch_stage1.zig

src/AstGen.zig+2-4

...	@@ -2109,7 +2109,6 @@ fn unusedResultExpr(gz: GenZir, scope: Scope, statement: Ast.Node.Index) Inner	...	@@ -2109,7 +2109,6 @@ fn unusedResultExpr(gz: GenZir, scope: Scope, statement: Ast.Node.Index) Inner
2109	.negate,	2109	.negate,
2110	.negate_wrap,	2110	.negate_wrap,
2111	.typeof,	2111	.typeof,
2112	.typeof_elem,
2113	.xor,	2112	.xor,
2114	.optional_type,	2113	.optional_type,
2115	.optional_payload_safe,	2114	.optional_payload_safe,
...	@@ -6028,8 +6027,7 @@ fn switchExpr(	...	@@ -6028,8 +6027,7 @@ fn switchExpr(
6028	const cond_tag: Zir.Inst.Tag = if (any_payload_is_ref) .switch_cond_ref else .switch_cond;	6027	const cond_tag: Zir.Inst.Tag = if (any_payload_is_ref) .switch_cond_ref else .switch_cond;
6029	const cond = try parent_gz.addUnNode(cond_tag, raw_operand, operand_node);	6028	const cond = try parent_gz.addUnNode(cond_tag, raw_operand, operand_node);
6030	// We need the type of the operand to use as the result location for all the prong items.	6029	// We need the type of the operand to use as the result location for all the prong items.
6031	const typeof_tag: Zir.Inst.Tag = if (any_payload_is_ref) .typeof_elem else .typeof;	6030	const cond_ty_inst = try parent_gz.addUnNode(.typeof, cond, operand_node);
6032	const cond_ty_inst = try parent_gz.addUnNode(typeof_tag, cond, operand_node);
6033	const item_rl: ResultLoc = .{ .ty = cond_ty_inst };	6031	const item_rl: ResultLoc = .{ .ty = cond_ty_inst };
6034		6032
6035	// These contain the data that goes into the `extra` array for the SwitchBlock/SwitchBlockMulti.	6033	// These contain the data that goes into the `extra` array for the SwitchBlock/SwitchBlockMulti.
...	@@ -6214,7 +6212,7 @@ fn switchExpr(	...	@@ -6214,7 +6212,7 @@ fn switchExpr(
6214	.has_multi_cases = multi_cases_len != 0,	6212	.has_multi_cases = multi_cases_len != 0,
6215	.has_else = special_prong == .@"else",	6213	.has_else = special_prong == .@"else",
6216	.has_under = special_prong == .under,	6214	.has_under = special_prong == .under,
6217	.scalar_cases_len = @intCast(u28, scalar_cases_len),	6215	.scalar_cases_len = @intCast(Zir.Inst.SwitchBlock.Bits.ScalarCasesLen, scalar_cases_len),
6218	},	6216	},
6219	});	6217	});
6220		6218

src/Sema.zig+22-22

 @@ -608,7 +608,6 @@ pub fn analyzeBody(
             .size_of                      => try sema.zirSizeOf(block, inst),
             .bit_size_of                  => try sema.zirBitSizeOf(block, inst),
             .typeof                       => try sema.zirTypeof(block, inst),
-            .typeof_elem                  => try sema.zirTypeofElem(block, inst),
             .log2_int_type                => try sema.zirLog2IntType(block, inst),
             .typeof_log2_int_type         => try sema.zirTypeofLog2IntType(block, inst),
             .xor                          => try sema.zirBitwise(block, inst, .xor),
 @@ -2337,11 +2336,21 @@ fn validateUnionInit(
         return sema.failWithBadUnionFieldAccess(block, union_obj, field_src, field_name);
     const field_index = @intCast(u32, field_index_big);
-    // TODO here we need to go back and see if we need to convert the union
+    // Handle the possibility of the union value being comptime-known.
-    // to a comptime-known value. This will involve editing the AIR code we have
+    const union_ptr_inst = Air.refToIndex(sema.resolveInst(field_ptr_extra.lhs)).?;
-    // generated so far - in particular deleting some runtime pointer bitcast
+    switch (sema.air_instructions.items(.tag)[union_ptr_inst]) {
-    // instructions which are not actually needed if the initialization expression
+        .constant => return, // In this case the tag has already been set. No validation to do.
-    // ends up being comptime-known.
+        .bitcast => {
+            // TODO here we need to go back and see if we need to convert the union
+            // to a comptime-known value. In such case, we must delete all the instructions
+            // added to the current block starting with the bitcast.
+            // If the bitcast result ptr is an alloc, the alloc should be replaced with
+            // a constant decl_ref.
+            // Otherwise, the bitcast should be preserved and a store instruction should be
+            // emitted to store the constant union value through the bitcast.
+        },
+        else => unreachable,
+    }
     // Otherwise, we set the new union tag now.
     const new_tag = try sema.addConstant(
 @@ -4091,18 +4100,20 @@ fn analyzeCall(
             zir_tags,
         );
     } else res: {
+        try sema.requireRuntimeBlock(block, call_src);
         const args = try sema.arena.alloc(Air.Inst.Ref, uncasted_args.len);
         for (uncasted_args) |uncasted_arg, i| {
+            const arg_src = call_src; // TODO: better source location
             if (i < fn_params_len) {
                 const param_ty = func_ty.fnParamType(i);
-                const arg_src = call_src; // TODO: better source location
+                try sema.resolveTypeLayout(block, arg_src, param_ty);
                 args[i] = try sema.coerce(block, param_ty, uncasted_arg, arg_src);
             } else {
                 args[i] = uncasted_arg;
             }
         }
-        try sema.requireRuntimeBlock(block, call_src);
         try sema.resolveTypeLayout(block, call_src, func_ty_info.return_type);
         try sema.air_extra.ensureUnusedCapacity(gpa, @typeInfo(Air.Call).Struct.fields.len +
 @@ -4173,6 +4184,7 @@ fn finishGenericCall(
                 const param_ty = new_fn_ty.fnParamType(runtime_i);
                 const arg_src = call_src; // TODO: better source location
                 const uncasted_arg = uncasted_args[total_i];
+                try sema.resolveTypeLayout(block, arg_src, param_ty);
                 const casted_arg = try sema.coerce(block, param_ty, uncasted_arg, arg_src);
                 runtime_args[runtime_i] = casted_arg;
                 runtime_i += 1;
 @@ -5548,7 +5560,7 @@ fn zirSwitchCapture(
                     );
                 }
                 try sema.requireRuntimeBlock(block, operand_src);
-                return block.addStructFieldPtr(operand_ptr, field_index, field.ty);
+                return block.addStructFieldPtr(operand_ptr, field_index, field_ty_ptr);
             }
             const operand = if (operand_is_ref)
 @@ -5669,11 +5681,7 @@ fn zirSwitchBlock(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError
     const special_prong_src: LazySrcLoc = .{ .node_offset_switch_special_prong = src_node_offset };
     const extra = sema.code.extraData(Zir.Inst.SwitchBlock, inst_data.payload_index);
-    const operand_ptr = sema.resolveInst(extra.data.operand);
+    const operand = sema.resolveInst(extra.data.operand);
-    const operand = if (extra.data.bits.is_ref)
-        try sema.analyzeLoad(block, src, operand_ptr, operand_src)
-    else
-        operand_ptr;
     var header_extra_index: usize = extra.end;
 @@ -8675,14 +8683,6 @@ fn zirTypeof(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Air.
     return sema.addType(operand_ty);
 }
-fn zirTypeofElem(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Air.Inst.Ref {
-    _ = block;
-    const inst_data = sema.code.instructions.items(.data)[inst].un_node;
-    const operand_ptr = sema.resolveInst(inst_data.operand);
-    const elem_ty = sema.typeOf(operand_ptr).elemType();
-    return sema.addType(elem_ty);
-}
 fn zirTypeofLog2IntType(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Air.Inst.Ref {
     const inst_data = sema.code.instructions.items(.data)[inst].un_node;
     const src = inst_data.src();

src/Zir.zig+11-6

 @@ -544,9 +544,6 @@ pub const Inst = struct {
         /// Returns the type of a value.
         /// Uses the `un_node` field.
         typeof,
-        /// Given a value which is a pointer, returns the element type.
-        /// Uses the `un_node` field.
-        typeof_elem,
         /// Given a value, look at the type of it, which must be an integer type.
         /// Returns the integer type for the RHS of a shift operation.
         /// Uses the `un_node` field.
 @@ -1045,7 +1042,6 @@ pub const Inst = struct {
                 .negate,
                 .negate_wrap,
                 .typeof,
-                .typeof_elem,
                 .xor,
                 .optional_type,
                 .optional_payload_safe,
 @@ -1312,7 +1308,6 @@ pub const Inst = struct {
                 .negate = .un_node,
                 .negate_wrap = .un_node,
                 .typeof = .un_node,
-                .typeof_elem = .un_node,
                 .typeof_log2_int_type = .un_node,
                 .log2_int_type = .un_node,
                 .@"unreachable" = .@"unreachable",
 @@ -2443,6 +2438,13 @@ pub const Inst = struct {
     ///        body member Index for every body_len
     ///    }
     pub const SwitchBlock = struct {
+        /// This is always a `switch_cond` or `switch_cond_ref` instruction.
+        /// If it is a `switch_cond_ref` instruction, bits.is_ref is always true.
+        /// If it is a `switch_cond` instruction, bits.is_ref is always false.
+        /// Both `switch_cond` and `switch_cond_ref` return a value, not a pointer,
+        /// that is useful for the case items, but cannot be used for capture values.
+        /// For the capture values, Sema is expected to find the operand of this operand
+        /// and use that.
         operand: Ref,
         bits: Bits,
 @@ -2454,8 +2456,11 @@ pub const Inst = struct {
             /// If true, there is an underscore prong. This is mutually exclusive with `has_else`.
             has_under: bool,
             /// If true, the `operand` is a pointer to the value being switched on.
+            /// TODO this flag is redundant with the tag of operand and can be removed.
             is_ref: bool,
-            scalar_cases_len: u28,
+            scalar_cases_len: ScalarCasesLen,
+            pub const ScalarCasesLen = u28;
             pub fn specialProng(bits: Bits) SpecialProng {
                 const has_else: u2 = @boolToInt(bits.has_else);

src/print_zir.zig-1

...	@@ -184,7 +184,6 @@ const Writer = struct {	...	@@ -184,7 +184,6 @@ const Writer = struct {
184	.is_non_err,	184	.is_non_err,
185	.is_non_err_ptr,	185	.is_non_err_ptr,
186	.typeof,	186	.typeof,
187	.typeof_elem,
188	.struct_init_empty,	187	.struct_init_empty,
189	.type_info,	188	.type_info,
190	.size_of,	189	.size_of,

src/type.zig+37-22

 @@ -3391,34 +3391,49 @@ pub const Type = extern union {
         }
     }
+    /// Supports structs and unions.
     pub fn structFieldOffset(ty: Type, index: usize, target: Target) u64 {
-        const fields = ty.structFields();
+        switch (ty.tag()) {
-        if (ty.castTag(.@"struct")) |payload| {
+            .@"struct" => {
-            const struct_obj = payload.data;
+                const struct_obj = ty.castTag(.@"struct").?.data;
-            assert(struct_obj.status == .have_layout);
+                assert(struct_obj.status == .have_layout);
-            const is_packed = struct_obj.layout == .Packed;
+                const is_packed = struct_obj.layout == .Packed;
-            if (is_packed) @panic("TODO packed structs");
+                if (is_packed) @panic("TODO packed structs");
-        }
-        var offset: u64 = 0;
+                var offset: u64 = 0;
-        var big_align: u32 = 0;
+                var big_align: u32 = 0;
-        for (fields.values()) |field, i| {
+                for (struct_obj.fields.values()) |field, i| {
-            if (!field.ty.hasCodeGenBits()) continue;
+                    if (!field.ty.hasCodeGenBits()) continue;
-            const field_align = a: {
+                    const field_align = a: {
-                if (field.abi_align.tag() == .abi_align_default) {
+                        if (field.abi_align.tag() == .abi_align_default) {
-                    break :a field.ty.abiAlignment(target);
+                            break :a field.ty.abiAlignment(target);
+                        } else {
+                            break :a @intCast(u32, field.abi_align.toUnsignedInt());
+                        }
+                    };
+                    big_align = @maximum(big_align, field_align);
+                    offset = std.mem.alignForwardGeneric(u64, offset, field_align);
+                    if (i == index) return offset;
+                    offset += field.ty.abiSize(target);
+                }
+                offset = std.mem.alignForwardGeneric(u64, offset, big_align);
+                return offset;
+            },
+            .@"union" => return 0,
+            .union_tagged => {
+                const union_obj = ty.castTag(.union_tagged).?.data;
+                const layout = union_obj.getLayout(target, true);
+                if (layout.tag_align >= layout.payload_align) {
+                    // {Tag, Payload}
+                    return std.mem.alignForwardGeneric(u64, layout.tag_size, layout.payload_align);
                 } else {
-                    break :a @intCast(u32, field.abi_align.toUnsignedInt());
+                    // {Payload, Tag}
+                    return 0;
                 }
-            };
+            },
-            big_align = @maximum(big_align, field_align);
+            else => unreachable,
-            offset = std.mem.alignForwardGeneric(u64, offset, field_align);
-            if (i == index) return offset;
-            offset += field.ty.abiSize(target);
         }
-        offset = std.mem.alignForwardGeneric(u64, offset, big_align);
-        return offset;
     }
     pub fn declSrcLoc(ty: Type) Module.SrcLoc {

test/behavior/switch.zig+43

...	@@ -219,3 +219,46 @@ test "switch on global mutable var isn't constant-folded" {	...	@@ -219,3 +219,46 @@ test "switch on global mutable var isn't constant-folded" {
219	poll();	219	poll();
220	}	220	}
221	}	221	}
		222
		223	const SwitchProngWithVarEnum = union(enum) {
		224	One: i32,
		225	Two: f32,
		226	Meh: void,
		227	};
		228
		229	test "switch prong with variable" {
		230	try switchProngWithVarFn(SwitchProngWithVarEnum{ .One = 13 });
		231	try switchProngWithVarFn(SwitchProngWithVarEnum{ .Two = 13.0 });
		232	try switchProngWithVarFn(SwitchProngWithVarEnum{ .Meh = {} });
		233	}
		234	fn switchProngWithVarFn(a: SwitchProngWithVarEnum) !void {
		235	switch (a) {
		236	SwitchProngWithVarEnum.One => \|x\| {
		237	try expect(x == 13);
		238	},
		239	SwitchProngWithVarEnum.Two => \|x\| {
		240	try expect(x == 13.0);
		241	},
		242	SwitchProngWithVarEnum.Meh => \|x\| {
		243	const v: void = x;
		244	_ = v;
		245	},
		246	}
		247	}
		248
		249	test "switch on enum using pointer capture" {
		250	try testSwitchEnumPtrCapture();
		251	comptime try testSwitchEnumPtrCapture();
		252	}
		253
		254	fn testSwitchEnumPtrCapture() !void {
		255	var value = SwitchProngWithVarEnum{ .One = 1234 };
		256	switch (value) {
		257	SwitchProngWithVarEnum.One => \|x\| x. += 1,
		258	else => unreachable,
		259	}
		260	switch (value) {
		261	SwitchProngWithVarEnum.One => \|x\| try expect(x == 1235),
		262	else => unreachable,
		263	}
		264	}

test/behavior/switch_stage1.zig-42

...	@@ -3,48 +3,6 @@ const expect = std.testing.expect;	...	@@ -3,48 +3,6 @@ const expect = std.testing.expect;
3	const expectError = std.testing.expectError;	3	const expectError = std.testing.expectError;
4	const expectEqual = std.testing.expectEqual;	4	const expectEqual = std.testing.expectEqual;
5		5
6	test "switch prong with variable" {
7	try switchProngWithVarFn(SwitchProngWithVarEnum{ .One = 13 });
8	try switchProngWithVarFn(SwitchProngWithVarEnum{ .Two = 13.0 });
9	try switchProngWithVarFn(SwitchProngWithVarEnum{ .Meh = {} });
10	}
11	const SwitchProngWithVarEnum = union(enum) {
12	One: i32,
13	Two: f32,
14	Meh: void,
15	};
16	fn switchProngWithVarFn(a: SwitchProngWithVarEnum) !void {
17	switch (a) {
18	SwitchProngWithVarEnum.One => \|x\| {
19	try expect(x == 13);
20	},
21	SwitchProngWithVarEnum.Two => \|x\| {
22	try expect(x == 13.0);
23	},
24	SwitchProngWithVarEnum.Meh => \|x\| {
25	const v: void = x;
26	_ = v;
27	},
28	}
29	}
30
31	test "switch on enum using pointer capture" {
32	try testSwitchEnumPtrCapture();
33	comptime try testSwitchEnumPtrCapture();
34	}
35
36	fn testSwitchEnumPtrCapture() !void {
37	var value = SwitchProngWithVarEnum{ .One = 1234 };
38	switch (value) {
39	SwitchProngWithVarEnum.One => \|x\| x. += 1,
40	else => unreachable,
41	}
42	switch (value) {
43	SwitchProngWithVarEnum.One => \|x\| try expect(x == 1235),
44	else => unreachable,
45	}
46	}
47
48	test "switch handles all cases of number" {	6	test "switch handles all cases of number" {
49	try testSwitchHandleAllCases();	7	try testSwitchHandleAllCases();
50	comptime try testSwitchHandleAllCases();	8	comptime try testSwitchHandleAllCases();