[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross added a comment. Is this just waiting for a review? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86310/new/ https://reviews.llvm.org/D86310 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross added a comment. In D86310#4504168 , @hvdijk wrote: > THE CURRENT STATE > [...] > Compatibility between LLVM and GCC > > For x86, the current i128 handling is compatible. The alignment to 8 byte > boundaries causes no compatibility issues because

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross added a comment. Here's confirmation that `_BitInt(128)` should be 8-byte aligned and not 16 (so, different from `__int128`) from https://gitlab.com/x86-psABIs/x86-64-ABI: > • For N > 64, they are treated as struct of 64-bit integer chunks. The number > of > chunks is the smallest numbe

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross added a comment. In D86310#4516911 , @hvdijk wrote: > In D86310#4516876 , @pengfei wrote: > >> Just FYI. There are a few reports about the compatibility issues, e.g., >> #41784

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross added a comment. @nikic posted a patch that fixes the register passing at https://reviews.llvm.org/D158169. I think that patch plus this one should resolve all the problems we have Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86310/new/

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross added a comment. Is clang still doing something wrong? From my testing, it seems like clang and GCC now agree with each other, I am not sure what would still be incorrect Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86310/new/ https://rev

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross added a comment. In D86310#4596730 , @hvdijk wrote: > My understanding is that the code clang generates for `__int128` will still > allow it to be passed half-in-register, half-in-memory, exactly what D158169 >

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross added a comment. Was your failure in https://bugs.llvm.org/show_bug.cgi?id=50198 fixed with these patches? I cannot reproduce that failure for some reason, but it would likely make a good run-pass test. These two patches do not seem to fix varargs segfaulting, as documented in https://

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross added a comment. In D86310#4597359 , @hvdijk wrote: >> I cannot reproduce that failure for some reason, but it would likely make a >> good run-pass test. > > It's reproducible online, https://godbolt.org/z/j918EeoMv, it would be > interesting to

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross added a comment. In D86310#4597359 , @hvdijk wrote: > In D86310#4596841 , @tmgross wrote: > >> I think that D158169 seems to have fixed >> clang as well; after applying bo

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross accepted this revision. tmgross added a comment. Tested that this patch applied on top of `main` fixes all i128 ABI issues among gcc, clang, and rustc. Probably would be good to add https://bugs.llvm.org/show_bug.cgi?id=50198 to the test suite if it isn't there already. Thanks for stic

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

tmgross added a comment. > See the source code comment I quoted in > https://bugs.llvm.org/show_bug.cgi?id=50198#c3: "If the target does not have > native f128 support, expand it to i128 and we will be generating soft float > library calls." This applies to x86. `f128` is expanded to `i128`, so

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

tmgross added a comment. Herald added a subscriber: wangpc. What is the current status of this patch? Are the reviewers here OK with this fix in general but just need to see changes to autoupgrade? @craig.topper or @hvdijk since you worked on it, are you interested in doing these changes, or is

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

tmgross added a comment. Thank you Craig and Harald for getting back so quick. I suppose that leaves it up to what level of `AutoUpgrade` changes would be accepted at a minimum. @efriedma would you consider the changes suggested by @hvdijk sufficient under any circumstances or would you still

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

tmgross added a comment. In D86310#4499095 , @rnk wrote: > I still think we're overthinking this, and letting our ABI compat concerns > block us from making progress. Maybe we could do something as simple as > erroring out from the auto-upgrader when the