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

[mschwaig]

This PR should resolve #2987 and make >= 128-bit integer types 16-byte aligned. The alignment and size of those types are both changed accordingly.

Some caveats:

For some reason inside get_llvm_type I cannot assert that integer types are sized and aligned the same as calculated by the functions I introduce here, so I had to exclude them form those assertions. I am not sure why that is. It would be good to know.

There is also no test included yet since the example test @andrewrk provided in #2987 is currently disabled because of #2883. I did run that test locally and it worked.

Maybe this could be covered with a test somehow by asserting how structs are padded.

[andrewrk]

Is the size one necessary? I believe LLVMABISizeOfType is returning the correct value.

[mschwaig]

I think I might have made a wrong assumption along the way which lead me to implementing the size one. I just checked and it is actually not required to pass the tests.

[andrewrk]

Can you add this test case?

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

I believe it will trip this assertion:

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

That assertion was useful while doing some big changes to the way llvm types were constructed, but now that Zig intentionally disagrees with LLVM about the alignment of types, the assertion should be retired (deleted).

[mschwaig]

I will look into that tomorrow. 👍

[mschwaig]

I fixed a bug in my implementation, now things make sense to me. I added some tests as well.

[andrewrk]

This assertion can stay, right?

[mschwaig]

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.

[andrewrk]

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

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

[mschwaig]

Done.

[andrewrk]

can you explain this change?

[mschwaig]

This prevents a segfault, probably due to alignment, that otherwise happens here (according to GDB):

testing.expect(context.data == thread_count * TestContext.incr_count);

[mschwaig]

I reverted this.

[andrewrk]

zig doesn't guarantee the size of non-extern, non-packed structs. You can fix this test case by making it extern struct.

[mschwaig]

I replaced this specific test with the other one you suggested above, since 129-bit integers are not allowed in extern structs which made this specific one obsolete.

[mschwaig]

I think there is some work left to do here.
Quite a few tests in the standard library are failing.

I fixed the Mutex one with a temporary hack, but I think there is some mismatch of expectations between LLVM and Zig when it comes to types that are presented as integers to LLVM but are some other type to Zig.

[andrewrk]

You could be hitting this:

zig/src/analyze.cpp

Lines 2271 to 2280 in 30466bc

	// TODO If we have no type_entry for the field, we've already failed to
	// compile the program correctly. This stage1 compiler needs a deeper
	// reworking to make this correct, or we can ignore the problem
	// and make sure it is fixed in stage2. This workaround is for when
	// there is a false positive of a dependency loop, of alignment depending
	// on itself. When this false positive happens we assume a pointer-aligned
	// field, which is usually fine but could be incorrectly over-aligned or
	// even under-aligned. See https://github.com/ziglang/zig/issues/1512
	} else if (field->type_entry == nullptr) {
	this_field_align = g->builtin_types.entry_usize->abi_align;

[andrewrk]

Sorry, I think I underestimated the difficulty of this one.

[mschwaig]

You could be hitting this:

zig/src/analyze.cpp

Lines 2271 to 2280 in 30466bc

	// TODO If we have no type_entry for the field, we've already failed to
	// compile the program correctly. This stage1 compiler needs a deeper
	// reworking to make this correct, or we can ignore the problem
	// and make sure it is fixed in stage2. This workaround is for when
	// there is a false positive of a dependency loop, of alignment depending
	// on itself. When this false positive happens we assume a pointer-aligned
	// field, which is usually fine but could be incorrectly over-aligned or
	// even under-aligned. See https://github.com/ziglang/zig/issues/1512
	} else if (field->type_entry == nullptr) {
	this_field_align = g->builtin_types.entry_usize->abi_align;

I do not think this is the issue. I verified this by increasing the default to twice the alignment, which I think should have made the problem go away again, as for types hitting this fall-back would have been sufficiently aligned then.

[mschwaig]

Update:
The same sefaults are still happening in the std tests.
I have added behavior tests for some of the cases I found here: 958d2a8

In all those cases an instruction is emitted to load a value from memory, for example

vmovdqa xmm0,XMMWORD PTR [rbp-0x1c8]

but the memory location does not satisfy the alignment requirements of the instruction, which leads to a segfault.

It looks like the beginnings of variables do not end up at a memory address which fits with their alignment.

[mschwaig]

In order to support 16-byte aligned integer types, types which contain them as fields would have to make sure that their fields alignment is respected. At runtime every field needs to end up at a memory location which is divisible by its alignment.

One way to achieve this is with the following two steps:

• Making the alignment of the container type the maximum alignment of of its fields (✔️) and
• arranging fields in a way that ensures each individual field has its alignment respected relative to the beginning of the container type. (❌)

I think the first one is satisfied, but the second one currently is not.

EDIT2: What is missing in Zig's implementation right now is support for leaving word-sized or larger gaps between members when required. Optionally fields could also be rearranged to minimize gaps.

EDIT: Also note that struct alignment still relies on LLVM types and not Zig types. I tried to fix this here, but ran into invalid LLVM which I was unable to fix.

[andrewrk]

I've got this fixed in #3033 (12ff91c)