Folkert de Vries (Jan 29 2024 at 20:57): Folkert de Vries (Jan 29 2024 at 20:57):Most list operations on lists of decimals will fail on x86 because of alignment https://github.com/roc-lang/roc/issues/6343 is an example
Folkert de Vries (Jan 29 2024 at 20:58):I can't think of a good way to fix it really
Richard Feldman (Jan 29 2024 at 21:03):do we know what LLVM release will fix that?
Folkert de Vries (Jan 29 2024 at 21:03):the next one I believe
Folkert de Vries (Jan 29 2024 at 21:11):I think https://reviews.llvm.org/D86310 is the implementation. it got merged, so it should be in LLVM 18
Folkert de Vries (Jan 29 2024 at 21:11):weirdly the (mostly empty) new release notes are for llvm 19
Brendan Hansknecht (Jan 30 2024 at 05:38):I am not sure I follow the issue fully. Don't we just need to change our internal alignment calculations to treat Dec as a {U64, U64} and set the alignment to 8 instead of 16. So just set this to 8: https://github.com/roc-lang/roc/blob/dd86b11150d4d33de842bf3fdb705bcd5409be15/crates/compiler/builtins/src/bitcode.rs#L134
Brendan Hansknecht (Jan 30 2024 at 05:39):Maybe we need to specialize on the OS too or similar?
Brendan Hansknecht (Jan 30 2024 at 05:41):Cause zig thinks an i128, is aligned to 8. So does roc llvm.
Brendan Hansknecht (Jan 30 2024 at 05:41):So probably do all of the platforms written in rust or c or zig.
Brendan Hansknecht (Jan 30 2024 at 05:46):Changing that one line fixes this specific issue on my x86_64 machine and causes the EIR.roc test to pass.
Brendan Hansknecht (Jan 30 2024 at 05:58):Just did testing. Rust and zig both see it as an align of 8 currently on x86_64. C/C++ technically don't have a native int128_t but there is the compiler builtin __int128_t which is 16 byte aligned (that said it isn't official and is not supported on most platforms).
Brendan Hansknecht (Jan 30 2024 at 05:59):Given all of our platforms are in rust or zig essentially and c++ can easily work around the alignment if it has a list of I128 or list of Dec, I think we should just change that to 8.
Brendan Hansknecht (Jan 30 2024 at 06:02):One other note, on 32bit targets, they consider i128 to be aligned to 4 bytes, so I guess that needs to be changed as well.
Brendan Hansknecht (Jan 30 2024 at 06:27): Folkert de Vries (Jan 30 2024 at 09:53):yeah the problem is how to make that consistent. We have many tests that rely on the ordering of fields. Changing the alignment on x86 but not on aarch causes field order to be different based on the target
Folkert de Vries (Jan 30 2024 at 09:54):so now you're sprinkling cfgs all over our tests, right?
Richard Feldman (Jan 30 2024 at 11:44):in terms of prioritization, I personally think it's fine to deprioritize this because if we wait for the next LLVM release (and the next Zig release which uses it) we'll just get the bugfix for free without putting any additional time into it :big_smile:
Brendan Hansknecht (Jan 30 2024 at 15:40):We need it anyway when we decide to properly handle 32bit systems. They aren't going to all change to align to 128bit.
Folkert de Vries (Jan 30 2024 at 15:40):I'd hope 32-bit x86 and arm use the same alignment though?
Brendan Hansknecht (Jan 30 2024 at 15:41):To my understanding only x86-64 is changing the alignment. 32bit X86 will still be aligned to 4. Not sure about 32bit arm I think it may have been 8.
Brendan Hansknecht (Jan 30 2024 at 15:47):Just double checked the llvm pr, they change both x86 and x86_64
Brendan Hansknecht (Jan 30 2024 at 15:48):But I think arm32 and wasm32 will still align i128 to 8 bytes.
Brendan Hansknecht (Jan 30 2024 at 16:05):in terms of prioritization, I personally think it's fine to deprioritize this because if we wait for the next LLVM release
Yeah, I thought this would be a real quick fix to unblock someone. That and, we actually need this change either way for arm32 and wasm32.
Brendan Hansknecht (Jan 30 2024 at 23:07):So I just did a bunch of checking with targets from rust nightly (which has already updated for the new llvm change)
Results: https://zig.godbolt.org/z/qd4jc7hfd
16 byte alignment: x86_64, x86_32, aarch64, risv64
8 byte alignment: wasm32, arm, thumb, risv32
So we are definitely going to have to deal with mono test record field order changes due to alignment in the long term. Same with any other tests that assume field order is the same on all architectures. That or add some sort of dummy sorting that will always put i128/u128/dec before i64/u64/f64 even if they have the same alignment (that actually might be a simpler fix for us). It could be a simple as sorting by alignment then size then name. Or just special casing those types.
In general, waiting for the llvm update will not fix everything, there is at least one extra part to deal with. Thoughts?
Richard Feldman (Jan 30 2024 at 23:15):so is llvm going to fix the rest too? :thinking:
Brendan Hansknecht (Jan 30 2024 at 23:21):The rest are correct
Brendan Hansknecht (Jan 30 2024 at 23:22):in x86 systems, the abi defines that 128bit integers are aligned to 16 bytes. Same with aarch64 and riscv64.
On the other systems, it is not defined that way.
Brendan Hansknecht (Jan 30 2024 at 23:22):So they really just updated to match the calling convention/abi.
Brendan Hansknecht (Jan 30 2024 at 23:32):Also, just to clarify the abi a bit more. I pretty sure it is the case that on the 32bit targets the largest value defined is a 64bit integer. As such, anything larger is required to be a struct. So an i128 is actually an {i64, i64}. Thus, it only has 8 byte alignment.
Richard Feldman (Jan 30 2024 at 23:34):ah interesting!
Richard Feldman (Jan 30 2024 at 23:34):so why would we want it to be 16 on those targets? :thinking:
Brendan Hansknecht (Jan 30 2024 at 23:36):We don't, we would prefer it to be aligned to 8. The problem arises that are tests aren't setup for it. Currently I don't think we test on any of those targets, but when we test one day on a 32bit arm machine some tests will fail.
Brendan Hansknecht (Jan 30 2024 at 23:36):For example any mono test with a record that has an I64 and I128 in it
Brendan Hansknecht (Jan 30 2024 at 23:36):The fields might reorder
Folkert de Vries (Jan 30 2024 at 23:41):I think that's mostly fine though. Mono should not depend on the target besides the field ordering. So most of mono itself should be adequately tested on a standard machine.
Brendan Hansknecht (Jan 30 2024 at 23:41):ah, so just don't run mono on those targets?
Folkert de Vries (Jan 30 2024 at 23:41):yeah
Folkert de Vries (Jan 30 2024 at 23:42):well, the (standard) tests
Folkert de Vries (Jan 30 2024 at 23:42):we could make some custom ones that actually test the alignment
Folkert de Vries (Jan 30 2024 at 23:42):but also, 32-bit systems are not that powerful, why would we even build the compiler on them in CI?
Brendan Hansknecht (Jan 30 2024 at 23:42):Ok. Then I say lets just set these to the correct values and then wait for llvm/zig/rust to update.
Brendan Hansknecht (Jan 30 2024 at 23:43):but also, 32-bit systems are not that powerful, why would we even build the compiler on them in CI?
True. Probably would do the same as we do for 32bit x86. Just run 32bit arm binary on a 64bit machine (I assume but am not actually sure if that is supported)
Folkert de Vries (Jan 30 2024 at 23:44):does that really test what you want to test?
Folkert de Vries (Jan 30 2024 at 23:44):cortex-m is different from cortex-a right?
Folkert de Vries (Jan 30 2024 at 23:44):maybe not in relevant ways though
Brendan Hansknecht (Jan 30 2024 at 23:45):It should at least test basic abi and alignment. llvm will deal with cpu specifics for us.
Brendan Hansknecht (Jan 30 2024 at 23:45):That said, I really don't care to build the compiler on 32bit arm. So maybe tests should just be about cross compiling being correct.
Brendan Hansknecht (Jan 30 2024 at 23:46):Anyway. I think we should submit this for now and just wait on llvm and such to catch up: #6474
Folkert de Vries (Jan 30 2024 at 23:47):yes, makes sense
Richard Feldman (Aug 03 2024 at 11:02):never thought I'd see the day when this issue actually closed! :heart_eyes:
https://github.com/rust-lang/rust/issues/54341#issuecomment-2266649613
Last updated: Jul 06 2025 at 12:14 UTC