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 any > changes to the alignment for `i128` automatically apply to `f128` as well.
Thank you for the explanation, that makes sense. > In D86310#4516911 <https://reviews.llvm.org/D86310#4516911>, @hvdijk wrote: > >> In D86310#4516876 <https://reviews.llvm.org/D86310#4516876>, @pengfei wrote: >> >>> There's also concern about the alignment difference between `_BitInt(128)` >>> and `__int128`, see #60925 >>> <https://github.com/llvm/llvm-project/issues/60925> >> >> That references https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/11, where >> the answer four months ago was basically "it's probably already too late for >> that" with a suggestion to try and post on the mailing list to try and >> convince others that this was important enough to do. Nothing was posted to >> the mailing list, and by now GCC has started implementing what the ABI >> specifies (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102989). I think we >> would need an extraordinary rationale if we want to convince others that the >> ABI should be changed. > > The discussion has since moved to the list > (https://groups.google.com/g/x86-64-abi/c/-JeR9HgUU20) and it seems as if the > alignment of `__int128` is fixed, no changes are planned there; if anything > changes, it will be the alignment of `_BitInt(128)`, and that will be > independent of this patch. Agreed; LLVM is doing the wrong thing with `i128` and the correct thing for `_BitInt(128)`, so `_BitInt` has no bearing on this change. > Based on this, I now do think again the right course of action is to just > commit this. It still applies to current LLVM without changes, and passes > tests. > > The point that is still contentious is the handling of IR generated from > older versions of LLVM that do not have this patch. Personally, I feel that > D158169 <https://reviews.llvm.org/D158169> being accepted already answered > how to handle this. D158169 <https://reviews.llvm.org/D158169> clearly broke > the ABI in LLVM: code generated with the current version of LLVM is not > binary compatible with code generated with older versions of LLVM. But that > is considered acceptable when the code generated by these older versions of > LLVM was buggy and we have no reason to expect that there is code out there > that relies on that bug remaining unfixed. The same logic applies here. Also agreed with this, I think concensus on this thread seems to be in agreement with the current patch too. Looking forward to the land :) 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.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
