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

Mon Sep 11 18:32:11 PDT 2023

hvdijk added a comment.

In D86310#4605475 <https://reviews.llvm.org/D86310#4605475>, @tmgross wrote:

> In D86310#4597359 <https://reviews.llvm.org/D86310#4597359>, @hvdijk wrote:
>
>> In D86310#4596841 <https://reviews.llvm.org/D86310#4596841>, @tmgross wrote:
>>
>>> I think that D158169 <https://reviews.llvm.org/D158169> seems to have fixed clang as well; after applying both patches, clang gcc and rustc all seem to agree.
>>
>> Interesting. I cannot see how it would, I may be missing something; I will check when I am able.
>
> D158169 <https://reviews.llvm.org/D158169> landed today, I confirmed that the current main (with D158169 <https://reviews.llvm.org/D158169>) makes Clang <-> GCC works but LLVM still fails without this patch.

I had hoped to avoid the piecewise ABI breakage, but with that already having landed, we already have that anyway, so I no longer see a reason to delay this until we can also fix `va_arg`.

> Doesn't clang just wind up going through the same tablegen as LLVM, so it makes sense that both would be fixed?

Actually able to look into this now again, and yes, it does. I was sure I'd seen clang expand `__int128` so that at the LLVM level, there was no longer any `i128`, but it does not happen here, and because it does not happen here, this patch does fix it.

>>> 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).
>
> You mean this patch only right - how does that work? Looking closer at your comments there, it doesn't seem like `i128` changes would affect anything if the `f128` return alignment is the source of the problem.

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.

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.

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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86310/new/

https://reviews.llvm.org/D86310