fix #49318, structs with odd-size primitive types should count as having padding #49362
Conversation
| @test sizeof(Tuple{UInt24}) == 4 | ||
| @test Base.datatype_haspadding(Tuple{UInt24}) | ||
| @test sizeof(UInt136) == 17 | ||
| @test sizeof(Tuple{UInt136}) == 24 |
There was a problem hiding this comment.
On 32 bit, I'd expect this to only be 20, since the base register size is only 32 bits there. Aligning 136 to the next largest multiple of 32 is 160, or 20 bytes.
EDIT: This is also the cause for the test failure here on CI:
This is the core issue with #49318 (and other padding related issues) - they are architecture specific. Conversely, on AVR sizeof(Tuple{UInt136}) ought to return 17 again (so no padding), due to the base register size there being single bytes (the code in this PR should do this, as far as I can tell - it's just the test that should be architecture specific as well).
There was a problem hiding this comment.
It should likely reflect the vector register alignment requirements from the ABI, which is likely supposed to be double-word size here (aside from clang bugs).
There was a problem hiding this comment.
At what size does the double-word alignment kick in?
There was a problem hiding this comment.
The clang compiler gets the ABI wrong, relative to other compilers and the standard in this. But i128 is supposed to be 128-bit aligned on most 64-bit platforms as an example (such as x86_64)
There was a problem hiding this comment.
It should likely reflect the vector register alignment requirements from the ABI, which is likely supposed to be double-word size here (aside from clang bugs).
Are you saying an i136 should end up effectively 256 bit aligned on x86_64..? And presumably that too on x86?
The clang compiler gets the ABI wrong, relative to other compilers and the standard in this.
How does it get it wrong? On 64 bit, i128 is supposed to be aligned to 2*64 = 128 already. On 32 bit, it's aligned to 4*32 = 128 as well, if I'm reading the standard right. Is that not what clang is already doing?
There was a problem hiding this comment.
By not having it set at all, clang kinda gets it right because they hard coded a fix somewhere, but LLVM in general gets it wrong. Fixing it is very easy, but it's an ABI break which is really annoying to handle.
There was a problem hiding this comment.
We have
# elif defined(_CPU_AARCH64_)
// int128 is 16 bytes aligned on aarch64
# define MAX_ALIGN 16
and 8 on other 64-bit systems. Should we change that?
There was a problem hiding this comment.
Not in this PR, but we are tracking that in another issue already
|
#58435) This is caused because for LLVMs sake we have to say that the oddly typed field is smaller than it actually is. (I wonder if we could represent it as an iN field in a struct and have it work but the result would be the same for now) Fix #58434, fix #49318, close #49362. --------- Co-authored-by: Mosè Giordano <[email protected]> Co-authored-by: Sukera <[email protected]> (cherry picked from commit 1b0b028)
#58435) This is caused because for LLVMs sake we have to say that the oddly typed field is smaller than it actually is. (I wonder if we could represent it as an iN field in a struct and have it work but the result would be the same for now) Fix #58434, fix #49318, close #49362. --------- Co-authored-by: Mosè Giordano <[email protected]> Co-authored-by: Sukera <[email protected]> (cherry picked from commit 1b0b028)
I believe our use of
MAX_ALIGNinjl_new_primitivetypealready computes the right alignment. This is intended to implement the C23 ABI for _BitInt.fixes #49318