search

JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.89k stars 5.49k forks source link

C compatible Int128 layout on x64 #20415

Closed yuyichao closed 3 months ago

yuyichao commented 7 years ago

A recent discourse post just reminds me about this....

(Or in general, any C->LLVM mapping that requires explicit padding.)

The alignment for Int128 on x64 is currently 8 in julia and LLVM. However, AFAIK, it should be 16 in order to match the C ABI. Fixing this may require adding explicit padding fields when we declare the LLVM struct in the same way clang does it. The hard part is to fix all the use of gep on a struct in codegen to use the actual field index including the padding field rather than the julia field index.

PallHaraldsson commented 7 years ago

https://reviews.llvm.org/D28990

simonbyrne commented 7 years ago

Will #22649 fix this?

yuyichao commented 7 years ago

Not afaict. The llvm pr might though.

vchuravy commented 7 years ago

The llvm PR got reverted in https://github.com/llvm-mirror/llvm/commit/8c54b816e554ff9f9a75ec8a1d50198d2bfd9633.

StefanKarpinski commented 7 years ago

I don't think this is a 1.0-blocking issue.

yuyichao commented 7 years ago

Note that fixing this will be breaking.

StefanKarpinski commented 7 years ago

Note that fixing this will be breaking.

Only if we declare struct layout to be stable in 1.0, which I don't think we should do.

yuyichao commented 7 years ago

What do you mean? The structure layout (and size) is a user observable API so changing that is breaking.

StefanKarpinski commented 7 years ago

Yes, but what we promise not to break is up to us.

yuyichao commented 7 years ago

Well, that's an API that people needs to rely on so we'll have to call that a breaking change, especially for isbits layout. Whether that can be an API breakage exception for 1.0 is another issue.

vtjnash commented 3 months ago

AFAIK, LLVM 18 fixed this?

vchuravy commented 3 months ago

We probably should add tests then?

I found quite nice to explain the situation. https://blog.rust-lang.org/2024/03/30/i128-layout-update.html