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

make >= 128-bit integer types 16-byte aligned #3003

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/analyze.cpp
Expand Up @@ -5627,6 +5627,13 @@ ZigType *make_int_type(CodeGen *g, bool is_signed, uint32_t size_in_bits) {
entry->llvm_type = LLVMIntType(size_in_bits);
entry->abi_size = LLVMABISizeOfType(g->target_data_ref, entry->llvm_type);
entry->abi_align = LLVMABIAlignmentOfType(g->target_data_ref, entry->llvm_type);
if (size_in_bits >= 128) {
// Override the alignment reported by LLVM like Clang does.
// On x86_64 there are some instructions like CMPXCHG16B which require this.
// See: https://github.com/ziglang/zig/issues/2987
assert(entry->abi_align == 8);
entry->abi_align = 16;
}
}

const char u_or_i = is_signed ? 'i' : 'u';
Expand Down Expand Up @@ -7266,8 +7273,6 @@ static void resolve_llvm_types(CodeGen *g, ZigType *type, ResolveStatus wanted_r

LLVMTypeRef get_llvm_type(CodeGen *g, ZigType *type) {
assertNoError(type_resolve(g, type, ResolveStatusLLVMFull));
assert(type->abi_size == 0 || type->abi_size == LLVMABISizeOfType(g->target_data_ref, type->llvm_type));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion can stay, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this holds anymore, because a larger alignment required for a field can increase the size of a struct.

It would be possible to assert

assert(type->abi_size == 0 || type->abi_size >= LLVMABISizeOfType(g->target_data_ref, type->llvm_type));

instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that makes sense. Here's another test case to add:

comptime {
    expect(@sizeOf(extern struct {
        x: u128,
        y: u8,
    }) == 32);
} 

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

assert(type->abi_align == 0 || type->abi_align == LLVMABIAlignmentOfType(g->target_data_ref, type->llvm_type));
return type->llvm_type;
}

Expand Down
63 changes: 63 additions & 0 deletions test/stage1/behavior/align.zig
Expand Up @@ -167,6 +167,14 @@ test "@ptrCast preserves alignment of bigger source" {
expect(@typeOf(ptr) == *align(16) u8);
}

fn give() anyerror!u128 {
return 3;
}

test "return error union with 128-bit integer" {
expect(3 == try give());
}

test "runtime known array index has best alignment possible" {
// take full advantage of over-alignment
var array align(4) = [_]u8{ 1, 2, 3, 4 };
Expand Down Expand Up @@ -220,6 +228,61 @@ test "alignment of structs" {
}) == @alignOf(usize));
}

test "alignment of >= 128-bit integer type" {
expect(@alignOf(u128) == 16);
expect(@alignOf(u129) == 16);
}

test "alignment of struct with 128-bit field" {
expect(@alignOf(struct {
x: u128,
}) == 16);

comptime {
expect(@alignOf(struct {
x: u128,
}) == 16);
}
}

test "size of extern struct with 128-bit field" {
expect(@sizeOf(extern struct {
x: u128,
y: u8,
}) == 32);

comptime {
expect(@sizeOf(extern struct {
x: u128,
y: u8,
}) == 32);
}
}

const DefaultAligned = struct {
nevermind: u32,
badguy: i128,
};

test "read 128-bit field from default aligned struct in stack memory" {
var default_aligned = DefaultAligned {
.nevermind = 1,
.badguy = 12,
};
expect((@ptrToInt(&default_aligned.badguy) % 16) == 0);
expect(12 == default_aligned.badguy);
}

var default_aligned_global = DefaultAligned {
.nevermind = 1,
.badguy = 12,
};

test "read 128-bit field from default aligned struct in global memory" {
expect((@ptrToInt(&default_aligned_global.badguy) % 16) == 0);
expect(12 == default_aligned_global.badguy);
}

test "alignment of extern() void" {
var runtime_nothing = nothing;
const casted1 = @ptrCast(*const u8, runtime_nothing);
Expand Down