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

Introduce DtoAlignment() and overload DtoAlloca() for VarDeclarations #1159

Merged
merged 4 commits into from
Oct 20, 2015

Conversation

kinke
Copy link
Member

@kinke kinke commented Oct 16, 2015

Trying to get the alignment right by using the first non-default one in the following order of descending priority:

VarDeclaration::alignment [variables only of course]
Type::alignment()
Type::alignsize()

This fixes the alignment of align(x) struct S { ... } instances on the stack.
Local variables with an explicit align attribute are still default-aligned though. :/

Trying to get the alignment right by using the first non-default one
in the following order of descending priority:

VarDeclaration::alignment [variables only of course]
Type::alignment()
Type::alignsize()

This fixes `align(x) struct S { ... }`.
@kinke
Copy link
Member Author

kinke commented Oct 19, 2015

An error in runnable/ldc_github_726.d seems to be caused by a wrongly determined size for a packed struct:

struct Buggy
{
    align(1):
    uint a = 0x0a0a0a0a;
    ulong b = 0x0b0b0b0b0b0b0b0b;
}

The offset is computed correctly in

Buggy packed;
ulong raw = *cast(ulong*)(cast(ubyte*)&packed + packed.b.offsetof);

and the LL type type { i32, i64, [4 x i8] } is the same as upstream. I think it's a separate bug that the determined size is 16 bytes; the max member alignment is 4, so I think the struct's should be 4 as well, leading to a size of 12 bytes. Apparently setting the struct alignment to 8, just because a member's alignment would be 8 if it wasn't packed, but is actually 4, seems wrong.

Anyway, with this PR, the size used when initializing packed by memcpying the (correct) initializer is 24 bytes.

Oh, I've just noted that the upstream IR uses a slightly different type: type <{ i32, i64, [4 x i8] }>, i.e., it encloses it in <>. Let me guess - it means packed, right? ;)

We need the type's natural alignment for that.
@kinke
Copy link
Member Author

kinke commented Oct 19, 2015

To verify the PR, it's useful to inspect the generated .ll for:

align(16) struct Struct { int[2] a; }
struct S { align(16) int a; }

void main()
{
    align(32) int[2] x;
    align(32) Struct s;
    S s2;
    //void foo() { x[0] = 666; s.a[0] = 123; }
    //foo();
}

=>

%align.Struct = type { [2 x i32], [8 x i8] }
%align.S = type { i32, [12 x i8] }
...
%x = alloca [2 x i32], align 4                  ; [#uses = 1, size/byte = 8]
%s = alloca %align.Struct, align 16             ; [#uses = 2, size/byte = 16]
%s2 = alloca %align.S, align 16                 ; [#uses = 1, size/byte = 16]

When uncommenting foo() and its invokation, x and s are captured and so placed in a nested context IR struct. LDC now adds an 8-bytes padding inbetween to make sure s is 16-bytes aligned, aligns the context struct itself at a 16-bytes boundary and uses the correct GEP index for the nested variable:

%nest.main = type { [2 x i32], [8 x i8], %align.Struct }
...
%.frame = alloca %nest.main, align 16           ; [#uses = 3, size/byte = 32]
%s2 = alloca %align.S, align 16                 ; [#uses = 1, size/byte = 16]
%x = getelementptr %nest.main, %nest.main* %.frame, i32 0, i32 0 ; [#uses = 1, type = [2 x i32]*]
%s = getelementptr %nest.main, %nest.main* %.frame, i32 0, i32 2 ; [#uses = 1, type = %align.Struct*]
... in foo():
%s = getelementptr %nest.main, %nest.main* %5, i32 0, i32 2 ; [#uses = 1, type = %align.Struct*]

What's still not working is the explicit alignment for local variables, i.e., align(32) is totally ignored.

@kinke
Copy link
Member Author

kinke commented Oct 19, 2015

https://github.com/ldc-developers/phobos/blob/ldc/std/conv.d#L5105 fails due to an insufficiently aligned (1-bytes on IR level, 4 bytes on the Travis boxes) static byte[] array when trying to emplace a class instance in it (8-bytes aligned). I need to fix the alignment of these globals.

…obals

This is apparently assumed by a std.conv unittest.

Using DtoAlignment() here instead of checking for an explicit
vd->alignment != STRUCTALIGN_DEFAULT is not only nice for consistency,
but also leads to a `struct S { align(16) int a; }` global being correctly
aligned on a 16-bytes boundary. The struct itself has no explicit
alignment, but alignsize() is 16; so the upstream code doesn't set any
explicit alignment for the global.
@kinke
Copy link
Member Author

kinke commented Oct 19, 2015

Wrt. the locals, I assumed their VarDeclaration::alignment would be set by the front-end if there was an align(x) attribute, but apparently that's not the case.

dnadlinger added a commit that referenced this pull request Oct 20, 2015
Introduce DtoAlignment() and overload DtoAlloca() for VarDeclarations
@dnadlinger dnadlinger merged commit 34529d6 into ldc-developers:master Oct 20, 2015
@MartinNowak
Copy link
Contributor

No tests?

I just tried to translate _mm_loadu_ps to D, but the compiler keeps using an aligned load.

import core.simd, ldc.simd;

align(1) struct Float4
{
    align(1) float4 v;
}

float4 test(float* ptr)
{
    return (cast(Float4*)ptr).v;
}

@JohanEngelen
Copy link
Member

@MartinNowak Tests are here: https://github.com/ldc-developers/ldc/blob/master/tests/codegen/align.d
But your case is missing.

(there are some "CHECK" things in that testfile (disabled tests), about which there should be a D language discussion)

@JohanEngelen
Copy link
Member

@MartinNowak Could you report a new issue for this?

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.

None yet

4 participants