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

[Trevor Gross via Phabricator via cfe-commits]

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