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

[Trevor Gross via Phabricator via cfe-commits]

tmgross added a comment.

In D86310#4516911 <https://reviews.llvm.org/D86310#4516911>, @hvdijk wrote:

> In D86310#4516876 <https://reviews.llvm.org/D86310#4516876>, @pengfei wrote:
>
>> Just FYI. There are a few reports about the compatibility issues, e.g., #41784 <https://github.com/llvm/llvm-project/issues/41784>.
>
> Thanks. This is a case where clang breaks up `__int128` into 2x `i64` before it gets to LLVM. It is therefore not affected by this patch. Your other link also references #20283 <https://github.com/llvm/llvm-project/issues/20283>, which is the same issue of clang breaking `__int128` into 2x `i64`.
>
> Although this patch will not fix those issues, it may make it easier to fix them later on: it will give clang the ability to use LLVM's `i128` type rather than trying to emulate it.>

Does this happen on the clang side or the LLVM side? I built rustc against LLVM with your patch ([link to source](llvm.org/docs/LangRef.html#floating-point-types)) and it makes rustc compatible with clang (progress!) but it still seems not compatible with GCC. That is, after the patch rustc now seems to have an identical calling behavior to clang, so I'm thinking that maybe this behavior lies somewhere in LLVM and not the frontend?

Quick ABI check that demonstrates this https://github.com/tgross35/quick-abi-check, the outputs of note (clang-new is built with this patch):

  # all caller-foo-callee-samefoo work fine
  
  + ./bins/caller-gcc-callee-gcc
  caller cc: gcc 11.3.0
  caller align i128 16
  caller arg0 244
  caller argval 0xf0e0d0c0b0a09080706050403020100
  caller arg15 123456.125000
  callee cc: gcc 11.3.0
  callee arg0 244
  callee arg1 0xf0e0d0c0b0a09080706050403020100
  callee arg2 0xf0e0d0c0b0a09080706050403020100
  callee arg3 0xf0e0d0c0b0a09080706050403020100
  callee arg4 0xf0e0d0c0b0a09080706050403020100
  callee arg15 123456.125000
  
  # between clang and gcc arg3+ seem to flip he word order?
  + ./bins/caller-gcc-callee-clang-old
  caller cc: gcc 11.3.0
  caller align i128 16
  caller arg0 244
  caller argval 0xf0e0d0c0b0a09080706050403020100
  caller arg15 123456.125000
  callee cc: clang 14.0.0 
  callee arg0 244
  callee arg1 0xf0e0d0c0b0a09080706050403020100
  callee arg2 0xf0e0d0c0b0a09080706050403020100
  callee arg3 0x7060504030201000000000000000000
  callee arg4 0x7060504030201000f0e0d0c0b0a0908
  callee arg15 123456.125000
  
  + ./bins/caller-gcc-callee-clang-new
  caller cc: gcc 11.3.0
  caller align i128 16
  caller arg0 244
  caller argval 0xf0e0d0c0b0a09080706050403020100
  caller arg15 123456.125000
  callee cc: clang 17.0.0 (git at github.com:tgross35/llvm-project.git 1733d949633a61cd0213f63e22d461a39e798946)
  callee arg0 244
  callee arg1 0xf0e0d0c0b0a09080706050403020100
  callee arg2 0xf0e0d0c0b0a09080706050403020100
  callee arg3 0x7060504030201000000000000000000
  callee arg4 0x7060504030201000f0e0d0c0b0a0908
  callee arg15 123456.125000

I think this patch can stand on its own even if it doesn't fix the above, but I'm just trying to get a better idea of where it's coming from if anyone knows more details.

>> 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.

I reached out to the person who authored that. We will follow up with the mailing list to at least make a mild push for `_BitInt(128)` to take the alignment of `__int128` (imagine the stackoverflow confusion if they aren't the same). However, I'm in agreement with the above comment - that is a separate concern from the behavior here since LLVM already complies with the bitint spec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310