-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Respect alignments above alignof(Void*)
inside union values
#14279
Respect alignments above alignof(Void*)
inside union values
#14279
Conversation
This patch is currently breaking upcasts like x = 1 || 1_i64
y = x.as(Int32 | Int64 | Int128) %x = alloca %"(Int32 | Int64)", align 8
%y = alloca %"(Int128 | Int32 | Int64)", align 8
%1 = alloca %"(Int128 | Int32 | Int64)", align 8
; ...
%5 = load %"(Int32 | Int64)", ptr %x, align 8
store %"(Int32 | Int64)" %5, ptr %1, align 8
%6 = load %"(Int128 | Int32 | Int64)", ptr %1, align 8
store %"(Int128 | Int32 | Int64)" %6, ptr %y, align 8
%7 = load %"(Int128 | Int32 | Int64)", ptr %1, align 8 In short, Downcasts fail for a similar reason. It seems sidecasts such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't dive into the details but as long as it works... looks good 👍
CI is failing on aarc64 though.
Given: a = uninitialized Int32 | Int64 | Int128
b = 1.as(Int32 | Int64)
a = b Master produces: %4 = load %"(Int32 | Int64)", ptr %b, align 8
store %"(Int32 | Int64)" %4, ptr %a, align 8 This PR currently produces: %4 = getelementptr inbounds %"(Int32 | Int64)", ptr %b, i32 0, i32 0
%5 = load i32, ptr %4, align 4
%6 = getelementptr inbounds %"(Int32 | Int64)", ptr %b, i32 0, i32 1
%7 = getelementptr inbounds %"(Int128 | Int32 | Int64)", ptr %a, i32 0, i32 0
store i32 %5, ptr %7, align 4
%8 = getelementptr inbounds %"(Int128 | Int32 | Int64)", ptr %a, i32 0, i32 1
%9 = getelementptr inbounds %"(Int32 | Int64)", ptr %b, i32 0, i32 1
%10 = load [1 x i64], ptr %9, align 8
store [1 x i64] %10, ptr %8, align 8 That last store doesn't look right; it probably needs to be ; ...
call void @llvm.memcpy.p0.p0.i64(ptr align 16 %8, ptr align 8 %9, i64 8, i1 false) |
Fixes the codegen issue in #14277 (comment).
If
alignof(Int128) == 16
(due to #13609 this is not true for x86-64 targets before LLVM 18), AnInt32 | Int128
will now place its type ID and data at offsets 0 and 16 respectively, including on AArch64, wherealignof(Int128) == 16
has always been the case. This also means that if, say, a 64-byte-aligned value becomes available in Crystal (e.g. for AVX-512), then a union of that value with anything else requires at least 128 bytes of storage, with 60 bytes of padding after the type ID.This is an ABI breaking change. It might be worth mentioning this even though Crystal makes no guarantees about ABI stability and most users won't be affected.
This doesn't cover the interpreter yet.