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

hvdijk added a comment. In D86310#4656024 , @Fznamznon wrote: > Hi there, > > This change seems to be causing assertion failure in clang when a struct > contains a _BitInt with length longer than 128 - > https://godbolt.org/z/4jTrW4fcP . Thanks for the

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

Fznamznon added a comment. Hi there, This change seems to be causing assertion failure in clang when a struct contains a _BitInt with length longer than 128 - https://godbolt.org/z/4jTrW4fcP . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

hvdijk added a comment. The buildbot found one more test that needed updating, that was disabled on my system. Created https://github.com/llvm/llvm-project/pull/68781 for that. 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

hvdijk added a comment. In D86310#4653550 , @tmgross wrote: > 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. That test would not work as an LLVM test directly, but we do already

[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

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

echristo accepted this revision. echristo added a comment. Explicitly still ok with this as well. Thanks for continuing here. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86310/new/ https://reviews.llvm.org/D86310

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

efriedma added a comment. Given the complexity here, I agree this is probably the best we can reasonably do. Code and test changes LGTM. That said, this is missing a release note. 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

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I re-read the code review, and I think most folks are in favor of this change, but I may have missed some. Many concerns were raised, so please wait for approval from @efriedma as well before

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

hvdijk added inline comments. Comment at: llvm/lib/IR/AutoUpgrade.cpp:5233 + SmallVector Groups; + Regex R("(.*-i64:64)(-.*)"); + if (R.match(Res, )) hvdijk wrote: > nikic wrote: > > I don't think this will work for the 32-bit targets that don't

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

rnk added a subscriber: akhuang. rnk added a comment. Regarding upgrade-datalayout2.ll, I don't think we need to be too constrained by it. @akhuang , do you recall why you added it? In other words, I think your direction is reasonable, we should go forward with this. Repository: rG LLVM

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

hvdijk added a comment. I do not think there is a sensible way to keep [`upgrade-datalayout2.ll`](https://github.com/llvm/llvm-project/blob/main/llvm/test/Bitcode/upgrade-datalayout2.ll) working, with the way the upgrade logic is structured, and we should rethink that test. The change here

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

hvdijk added inline comments. Comment at: llvm/lib/IR/AutoUpgrade.cpp:5233 + SmallVector Groups; + Regex R("(.*-i64:64)(-.*)"); + if (R.match(Res, )) nikic wrote: > I don't think this will work for the 32-bit targets that don't have `-i64:64`. Oh,

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

nikic added a comment. In D86310#4648646 , @hvdijk wrote: > In D86310#4648634 , @nikic wrote: > >> I'm happy to sign off on the x86-64 part here, but I'm less sure about >> x86-32. If I understood correctly, the

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

hvdijk added a comment. In D86310#4648634 , @nikic wrote: > I'm happy to sign off on the x86-64 part here, but I'm less sure about > x86-32. If I understood correctly, the i128 alignment is raised there > exclusively to fix the "f128 legalized to i128

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

nikic added a comment. I'm happy to sign off on the x86-64 part here, but I'm less sure about x86-32. If I understood correctly, the i128 alignment is raised there exclusively to fix the "f128 legalized to i128 libcall" case -- is there any other ABI requirement for i128 alignment on x86-32?

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

hvdijk added a comment. In D86310#4648446 , @tmgross wrote: > Is this just waiting for a review? Yes, I think so. Valid concerns over compatibility were raised, but now that strict compatibility with `i128` has already been broken anyway, I no longer

[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

[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`,

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

hvdijk added a comment. In D86310#4605475 , @tmgross wrote: > In D86310#4597359 , @hvdijk wrote: > >> In D86310#4596841 , @tmgross wrote: >> >>> I think that D158169

[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

[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

hvdijk added a comment. In D86310#4596932 , @tmgross wrote: > Was your failure in https://bugs.llvm.org/show_bug.cgi?id=50198 fixed with > these patches? Yes, it was (at least it was at the time that I initially commented). > I cannot reproduce that

[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

[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

hvdijk added a comment. In D86310#4596712 , @tmgross wrote: > 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 My understanding is that the code

[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/

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

hvdijk added a comment. In D86310#4595996 , @tmgross wrote: > @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 Thanks for the link,

[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

hvdijk added a comment. In D86310#4519549 , @tmgross wrote: > Does this happen on the clang side or the LLVM side? Definitely on the clang side, but... > I built rustc against LLVM with your patch ([link to >

[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

hvdijk added a comment. In D86310#4516876 , @pengfei wrote: > Just FYI. There are a few reports about the compatibility issues, e.g., > #41784 . Thanks. This is a case where clang breaks up `__int128`

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

pengfei added a comment. Just FYI. There are a few reports about the compatibility issues, e.g., #41784 . There's also concern about the alignment difference between `_BitInt(128)` and `__int128`, see #60925

[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

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

hvdijk added a comment. In D86310#4516184 , @tmgross wrote: > Is the compatibility note here only meant to address calls between LLVM and > GCC, not generated code? Because of course, struct layout and pass-in-memory > function calls are incompatible.

[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