From 3e30ba3f20dce2d406253de3fc0eb86934a3eaa7 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Tue, 7 Jun 2022 20:51:14 -0700 Subject: [PATCH 1/2] stage2: better codegen for byte-aligned packed struct fields * Sema: handle overaligned packed struct field pointers * LLVM: handle byte-aligned packed struct field pointers --- src/Sema.zig | 23 +++++++++++- src/codegen/llvm.zig | 40 ++++++++++++++------ src/type.zig | 21 +++++++++++ test/behavior/packed-struct.zig | 65 +++++++++++++++++++++++++++++++++ 4 files changed, 136 insertions(+), 13 deletions(-) diff --git a/src/Sema.zig b/src/Sema.zig index 5159d6f5d3..0cf449991d 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -18678,8 +18678,6 @@ fn structFieldPtrByIndex( const target = sema.mod.getTarget(); - // TODO handle when the struct pointer is overaligned, we should return a potentially - // over-aligned field pointer too. if (struct_obj.layout == .Packed) { comptime assert(Type.packed_struct_layout_version == 2); @@ -18700,6 +18698,27 @@ fn structFieldPtrByIndex( ptr_ty_data.host_size = struct_ptr_ty_info.host_size; ptr_ty_data.bit_offset += struct_ptr_ty_info.bit_offset; } + + const parent_align = if (struct_ptr_ty_info.@"align" != 0) + struct_ptr_ty_info.@"align" + else + struct_ptr_ty_info.pointee_type.abiAlignment(target); + ptr_ty_data.@"align" = parent_align; + + // If the field happens to be byte-aligned, simplify the pointer type. + // The pointee type bit size must match its ABI byte size so that loads and stores + // do not interfere with the surrounding packed bits. + if (parent_align != 0 and ptr_ty_data.bit_offset % 8 == 0) { + const byte_offset = ptr_ty_data.bit_offset / 8; + const elem_size_bytes = ptr_ty_data.pointee_type.abiSize(target); + const elem_size_bits = ptr_ty_data.pointee_type.bitSize(target); + if (elem_size_bytes * 8 == elem_size_bits) { + const new_align = @as(u32, 1) << @intCast(u5, @ctz(u64, byte_offset | parent_align)); + ptr_ty_data.bit_offset = 0; + ptr_ty_data.host_size = 0; + ptr_ty_data.@"align" = new_align; + } + } } else { ptr_ty_data.@"align" = field.abi_align; } diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index dcdf4888ea..188c2f6f11 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -8311,23 +8311,41 @@ pub const FuncGen = struct { field_index: u32, ) !?*const llvm.Value { if (self.liveness.isUnused(inst)) return null; + + const target = self.dg.object.target; const struct_ty = struct_ptr_ty.childType(); switch (struct_ty.zigTypeTag()) { .Struct => switch (struct_ty.containerLayout()) { .Packed => { - // From LLVM's perspective, a pointer to a packed struct and a pointer - // to a field of a packed struct are the same. The difference is in the - // Zig pointer type which provides information for how to mask and shift - // out the relevant bits when accessing the pointee. - // Here we perform a bitcast because we want to use the host_size - // as the llvm pointer element type. - const result_llvm_ty = try self.dg.lowerType(self.air.typeOfIndex(inst)); - // TODO this can be removed if we change host_size to be bits instead - // of bytes. - return self.builder.buildBitCast(struct_ptr, result_llvm_ty, ""); + const result_ty = self.air.typeOfIndex(inst); + const result_ty_info = result_ty.ptrInfo().data; + const result_llvm_ty = try self.dg.lowerType(result_ty); + + if (result_ty_info.host_size != 0) { + // From LLVM's perspective, a pointer to a packed struct and a pointer + // to a field of a packed struct are the same. The difference is in the + // Zig pointer type which provides information for how to mask and shift + // out the relevant bits when accessing the pointee. + // Here we perform a bitcast because we want to use the host_size + // as the llvm pointer element type. + return self.builder.buildBitCast(struct_ptr, result_llvm_ty, ""); + } + + // We have a pointer to a packed struct field that happens to be byte-aligned. + // Offset our operand pointer by the correct number of bytes. + const byte_offset = struct_ty.packedStructFieldByteOffset(field_index, target); + if (byte_offset == 0) { + return self.builder.buildBitCast(struct_ptr, result_llvm_ty, ""); + } + const llvm_bytes_ptr_ty = self.context.intType(8).pointerType(0); + const ptr_as_bytes = self.builder.buildBitCast(struct_ptr, llvm_bytes_ptr_ty, ""); + const llvm_usize = try self.dg.lowerType(Type.usize); + const llvm_index = llvm_usize.constInt(byte_offset, .False); + const indices: [1]*const llvm.Value = .{llvm_index}; + const new_ptr = self.builder.buildInBoundsGEP(ptr_as_bytes, &indices, indices.len, ""); + return self.builder.buildBitCast(new_ptr, result_llvm_ty, ""); }, else => { - const target = self.dg.module.getTarget(); var ty_buf: Type.Payload.Pointer = undefined; if (llvmFieldIndex(struct_ty, field_index, target, &ty_buf)) |llvm_field_index| { return self.builder.buildStructGEP(struct_ptr, llvm_field_index, ""); diff --git a/src/type.zig b/src/type.zig index 14c613a947..8e5eaf0ec7 100644 --- a/src/type.zig +++ b/src/type.zig @@ -5591,6 +5591,27 @@ pub const Type = extern union { } } + pub fn packedStructFieldByteOffset(ty: Type, field_index: usize, target: Target) u32 { + const struct_obj = ty.castTag(.@"struct").?.data; + assert(struct_obj.layout == .Packed); + comptime assert(Type.packed_struct_layout_version == 2); + + var bit_offset: u16 = undefined; + var running_bits: u16 = 0; + for (struct_obj.fields.values()) |f, i| { + if (!f.ty.hasRuntimeBits()) continue; + + if (i == field_index) { + bit_offset = running_bits; + } + running_bits += @intCast(u16, f.ty.bitSize(target)); + } + const host_size = (running_bits + 7) / 8; + _ = host_size; // TODO big-endian + const byte_offset = bit_offset / 8; + return byte_offset; + } + pub const FieldOffset = struct { field: usize, offset: u64, diff --git a/test/behavior/packed-struct.zig b/test/behavior/packed-struct.zig index 73c71ddf43..ed06cc486d 100644 --- a/test/behavior/packed-struct.zig +++ b/test/behavior/packed-struct.zig @@ -1,5 +1,6 @@ const std = @import("std"); const builtin = @import("builtin"); +const assert = std.debug.assert; const expect = std.testing.expect; const expectEqual = std.testing.expectEqual; const native_endian = builtin.cpu.arch.endian(); @@ -305,3 +306,67 @@ test "regular in irregular packed struct" { try expectEqual(@as(u16, 235), foo.bar.a); try expectEqual(@as(u8, 42), foo.bar.b); } + +test "byte-aligned field pointer offsets" { + if (builtin.zig_backend == .stage1) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_wasm) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; + + const S = struct { + const A = packed struct { + a: u8, + b: u8, + c: u8, + d: u8, + }; + + const B = packed struct { + a: u16, + b: u16, + }; + + fn doTheTest() !void { + var a: A = .{ + .a = 1, + .b = 2, + .c = 3, + .d = 4, + }; + comptime assert(@TypeOf(&a.a) == *align(4) u8); + comptime assert(@TypeOf(&a.b) == *u8); + comptime assert(@TypeOf(&a.c) == *align(2) u8); + comptime assert(@TypeOf(&a.d) == *u8); + try expect(a.a == 1); + try expect(a.b == 2); + try expect(a.c == 3); + try expect(a.d == 4); + a.a += 1; + a.b += 1; + a.c += 1; + a.d += 1; + try expect(a.a == 2); + try expect(a.b == 3); + try expect(a.c == 4); + try expect(a.d == 5); + + var b: B = .{ + .a = 1, + .b = 2, + }; + comptime assert(@TypeOf(&b.a) == *align(4) u16); + comptime assert(@TypeOf(&b.b) == *u16); + try expect(b.a == 1); + try expect(b.b == 2); + b.a += 1; + b.b += 1; + try expect(b.a == 2); + try expect(b.b == 3); + } + }; + + try S.doTheTest(); + comptime try S.doTheTest(); +} From 53c86febcbeee858e9c6536c54adf99ee644147e Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Tue, 7 Jun 2022 22:47:08 -0700 Subject: [PATCH 2/2] stage2: packed struct fixes for big-endian targets --- src/Sema.zig | 11 +++++-- src/type.zig | 7 ++-- test/behavior/packed-struct.zig | 57 ++++++++++++++++++++++++++------- 3 files changed, 58 insertions(+), 17 deletions(-) diff --git a/src/Sema.zig b/src/Sema.zig index 0cf449991d..8ce9226bd4 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -18708,11 +18708,18 @@ fn structFieldPtrByIndex( // If the field happens to be byte-aligned, simplify the pointer type. // The pointee type bit size must match its ABI byte size so that loads and stores // do not interfere with the surrounding packed bits. - if (parent_align != 0 and ptr_ty_data.bit_offset % 8 == 0) { - const byte_offset = ptr_ty_data.bit_offset / 8; + // We do not attempt this with big-endian targets yet because of nested + // structs and floats. I need to double-check the desired behavior for big endian + // targets before adding the necessary complications to this code. This will not + // cause miscompilations; it only means the field pointer uses bit masking when it + // might not be strictly necessary. + if (parent_align != 0 and ptr_ty_data.bit_offset % 8 == 0 and + target.cpu.arch.endian() == .Little) + { const elem_size_bytes = ptr_ty_data.pointee_type.abiSize(target); const elem_size_bits = ptr_ty_data.pointee_type.bitSize(target); if (elem_size_bytes * 8 == elem_size_bits) { + const byte_offset = ptr_ty_data.bit_offset / 8; const new_align = @as(u32, 1) << @intCast(u5, @ctz(u64, byte_offset | parent_align)); ptr_ty_data.bit_offset = 0; ptr_ty_data.host_size = 0; diff --git a/src/type.zig b/src/type.zig index 8e5eaf0ec7..b6f8db4ca1 100644 --- a/src/type.zig +++ b/src/type.zig @@ -5597,17 +5597,18 @@ pub const Type = extern union { comptime assert(Type.packed_struct_layout_version == 2); var bit_offset: u16 = undefined; + var elem_size_bits: u16 = undefined; var running_bits: u16 = 0; for (struct_obj.fields.values()) |f, i| { if (!f.ty.hasRuntimeBits()) continue; + const field_bits = @intCast(u16, f.ty.bitSize(target)); if (i == field_index) { bit_offset = running_bits; + elem_size_bits = field_bits; } - running_bits += @intCast(u16, f.ty.bitSize(target)); + running_bits += field_bits; } - const host_size = (running_bits + 7) / 8; - _ = host_size; // TODO big-endian const byte_offset = bit_offset / 8; return byte_offset; } diff --git a/test/behavior/packed-struct.zig b/test/behavior/packed-struct.zig index ed06cc486d..2483bbea69 100644 --- a/test/behavior/packed-struct.zig +++ b/test/behavior/packed-struct.zig @@ -289,13 +289,7 @@ test "regular in irregular packed struct" { const Irregular = packed struct { bar: Regular = Regular{}, - - // This field forces the regular packed struct to be a part of single u48 - // and thus it all gets represented as an array of 6 bytes in LLVM _: u24 = 0, - - // This struct on its own can represent its fields directly in LLVM - // with no need to use array of bytes as underlaying representation. pub const Regular = packed struct { a: u16 = 0, b: u8 = 0 }; }; @@ -335,17 +329,44 @@ test "byte-aligned field pointer offsets" { .c = 3, .d = 4, }; - comptime assert(@TypeOf(&a.a) == *align(4) u8); - comptime assert(@TypeOf(&a.b) == *u8); - comptime assert(@TypeOf(&a.c) == *align(2) u8); - comptime assert(@TypeOf(&a.d) == *u8); + switch (comptime builtin.cpu.arch.endian()) { + .Little => { + comptime assert(@TypeOf(&a.a) == *align(4) u8); + comptime assert(@TypeOf(&a.b) == *u8); + comptime assert(@TypeOf(&a.c) == *align(2) u8); + comptime assert(@TypeOf(&a.d) == *u8); + }, + .Big => { + // TODO re-evaluate packed struct endianness + comptime assert(@TypeOf(&a.a) == *align(4:0:4) u8); + comptime assert(@TypeOf(&a.b) == *align(4:8:4) u8); + comptime assert(@TypeOf(&a.c) == *align(4:16:4) u8); + comptime assert(@TypeOf(&a.d) == *align(4:24:4) u8); + }, + } try expect(a.a == 1); try expect(a.b == 2); try expect(a.c == 3); try expect(a.d == 4); + a.a += 1; + try expect(a.a == 2); + try expect(a.b == 2); + try expect(a.c == 3); + try expect(a.d == 4); + a.b += 1; + try expect(a.a == 2); + try expect(a.b == 3); + try expect(a.c == 3); + try expect(a.d == 4); + a.c += 1; + try expect(a.a == 2); + try expect(a.b == 3); + try expect(a.c == 4); + try expect(a.d == 4); + a.d += 1; try expect(a.a == 2); try expect(a.b == 3); @@ -356,11 +377,23 @@ test "byte-aligned field pointer offsets" { .a = 1, .b = 2, }; - comptime assert(@TypeOf(&b.a) == *align(4) u16); - comptime assert(@TypeOf(&b.b) == *u16); + switch (comptime builtin.cpu.arch.endian()) { + .Little => { + comptime assert(@TypeOf(&b.a) == *align(4) u16); + comptime assert(@TypeOf(&b.b) == *u16); + }, + .Big => { + comptime assert(@TypeOf(&b.a) == *align(4:0:4) u16); + comptime assert(@TypeOf(&b.b) == *align(4:16:4) u16); + }, + } try expect(b.a == 1); try expect(b.b == 2); + b.a += 1; + try expect(b.a == 2); + try expect(b.b == 2); + b.b += 1; try expect(b.a == 2); try expect(b.b == 3);