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

[Harald van Dijk via Phabricator via cfe-commits]

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 report. This is a pre-existing problem for `i128:128` targets -- 
the same assertion failure can be seen, without this change, for the same 
program with e.g. `--target=aarch64 -Xclang 
-fexperimental-max-bitint-width=1024` -- so I don't think it's a problem in 
this change directly, but considering X86 is the only target that enables 
>128-bit bit integers by default, it became far more visible after this change 
and because of that, more important to fix.


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

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

[Mariya Podchishchaeva via Phabricator via cfe-commits]

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

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

[Harald van Dijk via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Harald van Dijk via Phabricator via cfe-commits]

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 have lit 
tests that cover that, the test changes in here show the fixed alignment of 
f128 too.

This was ready to push pending @efriedma's approval, who rightly pointed out a 
release note was missing but it was otherwise okay. With the release note now 
added, I think that there is nothing stopping this from being pushed, so I 
intend to do so once I am able to rebase one hopefully last time and re-run 
tests to verify no new tests have been added that also require an update. 
Thanks for the feedback, everyone.


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

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

[Trevor Gross via Phabricator via cfe-commits]

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 sticking with this Harald!


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

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

[Eric Christopher via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eli Friedman via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via Phabricator via cfe-commits]

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


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

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

[Harald van Dijk via Phabricator via cfe-commits]

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 have 
> > `-i64:64`.
> Oh, you're right, thanks. That was intentional but wrong: there is a test 
> that we do not upgrade data layout strings that do not look sufficiently 
> close to valid, and this was intended to address that. But this also avoids 
> it for data layout strings that do need upgrading. I'll have to figure out 
> how to handle both; will update when I know how.
This should now be fixed. X86 data layout strings always have their components 
in the same order, `mpifnaS`, where some may be omitted. I make use of this by 
looking for any leading `-m`/`-p`/`-i` components and inserting `-i128:128` 
after the last of those.


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

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

[Reid Kleckner via Phabricator via cfe-commits]

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

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

[Harald van Dijk via Phabricator via cfe-commits]

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 intends to insert `-i128:128-` into x86 data layouts 
that do not have it. The goal of `upgrade-datalayout2.ll` is to test that data 
layouts that are not valid x86 data layouts do not get upgraded. However, I see 
no sensible logic by which we can say that in this particular case, we should 
not add it.

What's more, none of the data layout upgrades *ever* checked that the data 
layout was a valid x86 data layout, not even D67631 
 which added this test: it is easy to 
construct data layout strings that are not valid x86 data layout strings, that 
would already be upgraded by that very first version of 
`UpgradeDataLayoutString`, despite what the test claimed to check. So if we 
regard it as a bug to upgrade invalid target data layout strings, this is a 
pre-existing bug. Alternatively, we can choose to not regard it as a bug, and 
instead say the test is invalid. I do not know the rationale here, but given 
that it was explicitly said to be intended to work this way, I am on the side 
of seeing it as a pre-existing bug. One that is nearly impossible to fix in the 
current structure.

Now that there can only be one valid data layout string per target, if it is 
intended that `UpgradeDataLayoutString` only upgrade target-valid data layout 
strings, it is a bug for `UpgradeDataLayoutString` to ever produce anything 
other than 1) its input or 2) the target's one valid data layout string. This 
allows a much simpler implementation that completely fixes the bug, but is too 
big to be part of this change. I would like to propose that in this change, we 
change `UpgradeDataLayoutString` to insert `-i128:128-` including in that one 
test, and we XFAIL `upgrade-datalayout2.ll` since the uncovered bug is not 
actually a new bug. In a followup PR, I can then restructure the 
`UpgradeDataLayoutString` logic by removing the function entirely and instead 
having target functions to check whether a given data layout string is a valid 
historic data layout string for the target that should be upgraded, and if so, 
simply clobbering the data layout string with what the target reports is the 
correct data layout string.

Does that seem reasonable? Am I overlooking anything that would make this a 
non-option? Are there good alternatives that I am not seeing right now?


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

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

[Harald van Dijk via Phabricator via cfe-commits]

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, you're right, thanks. That was intentional but wrong: there is a test that 
we do not upgrade data layout strings that do not look sufficiently close to 
valid, and this was intended to address that. But this also avoids it for data 
layout strings that do need upgrading. I'll have to figure out how to handle 
both; will update when I know how.


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

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

[Nikita Popov via Phabricator via cfe-commits]

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 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? Is raising i128 
>> alignment the right way to fix an f128 issue?
>
> GCC does not support `__int128` on x86-32, but clang has the 
> `-fforce-enable-int128` option, and when that is used, it gives it the same 
> 16-byte alignment that it does on x86-64, so even ignoring the `_Float128` 
> issue, I think the change is right for x86-32.

Okay, that's a compelling argument.




Comment at: llvm/lib/IR/AutoUpgrade.cpp:5233
+  SmallVector Groups;
+  Regex R("(.*-i64:64)(-.*)");
+  if (R.match(Res, ))

I don't think this will work for the 32-bit targets that don't have `-i64:64`.


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

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

[Harald van Dijk via Phabricator via cfe-commits]

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 libcall" case -- is there any 
> other ABI requirement for i128 alignment on x86-32? Is raising i128 alignment 
> the right way to fix an f128 issue?

GCC does not support `__int128` on x86-32, but clang has the 
`-fforce-enable-int128` option, and when that is used, it gives it the same 
16-byte alignment that it does on x86-64, so even ignoring the `_Float128` 
issue, I think the change is right for x86-32.


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

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

[Nikita Popov via Phabricator via cfe-commits]

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? Is raising i128 alignment the right 
way to fix an f128 issue?


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

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

[Harald van Dijk via Phabricator via cfe-commits]

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 
believe there is any reason not to just apply this as is, preferably soon so as 
to minimise the time that we have the current partially changed `i128` ABI, to 
minimise the chance that people will start to rely on that somewhere.


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

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

[Trevor Gross via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 , @hvdijk wrote:
>
>> In D86310#4516876 , @pengfei wrote:
>>
>>> There's also concern about the alignment difference between `_BitInt(128)` 
>>> and `__int128`, see #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  being accepted already answered 
> how to handle this. 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

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

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4605475 , @tmgross wrote:

> In D86310#4597359 , @hvdijk wrote:
>
>> In D86310#4596841 , @tmgross wrote:
>>
>>> I think that 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  landed today, I confirmed that the 
> current main (with 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 , @hvdijk wrote:

> In D86310#4516876 , @pengfei wrote:
>
>> There's also concern about the alignment difference between `_BitInt(128)` 
>> and `__int128`, see #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 
 being accepted already answered how to 
handle this. 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Trevor Gross via Phabricator via cfe-commits]

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 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  landed today, I confirmed that the 
current main (with D158169 ) makes Clang <-> 
GCC works but LLVM still fails without this patch.

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

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

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.


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

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

[Trevor Gross via Phabricator via cfe-commits]

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 know why it does not fail for you.

I tested both on my machine and in a container (debian docker image, then 
installing `clang` and `gcc-multilib` only) and can't get it to reproduce. 
Weird.

  $ clang --version
  Ubuntu clang version 14.0.0-1ubuntu1.1
  Target: x86_64-pc-linux-gnu
  Thread model: posix
  InstalledDir: /usr/bin
  $ cat stack-fail.c
  int main(void) {
long double ld = 0;
__float128 f128 = ld;
int i = f128;
return i;
  }
  $ clang -m32 stack-fail.c && ./a.out && echo ok
  ok

F28728037: stack-fail.s 
F28728038: stack-fail.ll 

The IR looks about the same as on godbolt but maybe the attributes are 
affecting something. It's probably still doing the wrong thing, just not 
segfaulting for whatever reason

>> These two patches do not seem to fix varargs segfaulting, as documented in 
>> https://bugs.llvm.org/show_bug.cgi?id=19909 (testing with this code 
>> https://godbolt.org/z/WeE7TvrGe) so it seems like that will need a separate 
>> fix.
>
> Thanks, and clang appears to avoid the use of the LLVM `va_arg` instruction 
> here; we'll have to make sure to adapt that example to the LLVM IR equivalent 
> that does use `va_arg` to make sure that's tested as well, and fixed if 
> needed.

Are you comfortable landing these two patches without fixing varargs, since it 
seems like those need separate work? Not sure if llvm follows kernel 
conventions but assuming yes and assuming you are OK with however D158169 
 seems to fix clang, you can add `Tested-by: 
Trevor Gross `: to the best of my knowledge the alignment, 
ABI, and general interop problems on x86_64 have been resolved for both LLVM 
and Clang.


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

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

[Harald van Dijk via Phabricator via cfe-commits]

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 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 know why it does not fail for you.

> These two patches do not seem to fix varargs segfaulting, as documented in 
> https://bugs.llvm.org/show_bug.cgi?id=19909 (testing with this code 
> https://godbolt.org/z/WeE7TvrGe) so it seems like that will need a separate 
> fix.

Thanks, and clang appears to avoid the use of the LLVM `va_arg` instruction 
here; we'll have to make sure to adapt that example to the LLVM IR equivalent 
that does use `va_arg` to make sure that's tested as well, and fixed if needed.


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

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

[Trevor Gross via Phabricator via cfe-commits]

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 
https://bugs.llvm.org/show_bug.cgi?id=19909 (testing with this code 
https://godbolt.org/z/WeE7TvrGe) so it seems like that will need a separate fix.


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

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

[Trevor Gross via Phabricator via cfe-commits]

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 
>  sets out to fix, because D158169 
>  only fixes it for LLVM's `i128` which 
> clang bypasses.

I think that D158169  seems to have fixed 
clang as well; after applying both patches, clang gcc and rustc all seem to 
agree. On the readme for https://github.com/tgross35/quick-abi-check look at 
the tests `i128-caller-gcc-callee-clang-old` (args don't align) 
`i128-caller-gcc-callee-clang-new` (args **are** the same) and 
`i128-caller-gcc-callee-rustc` (args are the same). Also the full ABI checker 
seems to say everything is in order 
(https://github.com/rust-lang/rust/pull/113880#issuecomment-1683021483 not sure 
why it says "4 failed" at the end, but I think it's a bug since no tests 
actually show failed).

Does this all seem correct? As far as I can tell it seems like with both patchs 
these issues should be resolved.


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

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

[Harald van Dijk via Phabricator via cfe-commits]

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 clang generates for `__int128` will still 
allow it to be passed half-in-register, half-in-memory, exactly what D158169 
 sets out to fix, because D158169 
 only fixes it for LLVM's `i128` which clang 
bypasses.


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

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

[Trevor Gross via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Harald van Dijk via Phabricator via cfe-commits]

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, that will save a lot of time. I don't think it will 
resolve all the problems, but that it's a significant additional step in the 
right direction. We also need to make clang use `i128` for `__int128` in order 
to actually use this fixed calling convention.


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

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

[Trevor Gross via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D86310

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Harald van Dijk via Phabricator via cfe-commits]

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

...this suggests that potentially the same thing that clang is doing, LLVM is 
also doing independently. In which case, maybe it would be better to fix that 
at the same time: if we decide that LLVM's `i128` should match `__int128`, I'd 
rather have a single change to the ABI to make it match `__int128`, rather than 
incremental changes, because incremental changes make it more likely that 
someone is going to be using the intermediate version and relying on its ABI. 
It'll be a little while before I can look into this but I'll try to come up 
with a patch to apply on top of this if no one else gets to it first.

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

Thanks, this is useful as an extra test.


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

[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 , @hvdijk wrote:

> 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` into 2x `i64` before 
> it gets to LLVM. It is therefore not affected by this patch. Your other link 
> also references #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 0x706050403020100
  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 (g...@github.com:tgross35/llvm-project.git 
1733d949633a61cd0213f63e22d461a39e798946)
  callee arg0 244
  callee arg1 0xf0e0d0c0b0a09080706050403020100
  callee arg2 0xf0e0d0c0b0a09080706050403020100
  callee arg3 0x706050403020100
  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 
>> 
>
> 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Harald van Dijk via Phabricator via cfe-commits]

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` into 2x `i64` before it 
gets to LLVM. It is therefore not affected by this patch. Your other link also 
references #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.

> There's also concern about the alignment difference between `_BitInt(128)` 
> and `__int128`, see #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.


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

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

[Phoebe Wang via Phabricator via cfe-commits]

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 



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

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

[Trevor Gross via Phabricator via cfe-commits]

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 number that can contain the type. _BitInt(N) types are
> byte-aligned to 64 bits. The size of these types is the smallest multiple of 
> the 64-bit
> chunks greater than or equal to N.


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

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

[Harald van Dijk via Phabricator via cfe-commits]

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.

There should be no compatibility issue there between GCC and clang in most 
cases, because clang ensures __int128 is aligned to 16 bytes everywhere, even 
if the LLVM data layout specifies lower alignment. clang's __int128 and LLVM's 
i128 play by different rules, currently. This change would make them play by 
the same rules.

> Rust just uses LLVM's `i128` value directly so it doesn't necessarily need to 
> be called out on its own (think we are in agreement here, just clarifying)

I do think it needs to be called out on its own: Rust makes its

>> [...]
>> QUESTIONS
>>
>> Is the behaviour of LLVM, clang, GCC, and MSVC indeed as I described, or did 
>> I make a mistake anywhere?
>
> I believe that MSVC is in general ambiguous about these details on types that 
> it does not support, but I would assume that being consistent with the Linux 
> ABI is preferred and probably what MSVC would choose if they ever do decide 
> on a specification for this type (unless LLVM has contact with Microsoft that 
> may be able to clarify? They make no guarantees against breaking things in 
> any case.)
>
>> [...]
>
> It probably makes sense to have reasoning for choosing the selected behavior 
> and having something specific to test against, so I'll link what I know.
>
> - From AMD4 ABI Draft 0.99.7 (2014) 
> :
>
>> [paraphrased from Figure 3.1]
>> type - sizeof - alignment - AMD64 architecture
>> long - 8 - 8 - signed eightbyte [I included this in the table for the below 
>> reference]
>> `__int128` - 16 - 16 - signed sixteenbyte
>> signed `__int128` - 16 - 16 - signed sixteenbyte
>> `long double` - 16 - 16 - 80-bit extended (IEEE-754)
>> `__float128` - 16 - 16 - 128-bit extended (IEEE-754)
>> [...]
>> The `__int128` type is stored in little-endian order in memory, i.e., the 64
>> low-order bits are stored at a a lower address than the 64 high-order bits
>> [...]
>> Arguments of type `__int128` offer the same operations as INTEGERs,
>> yet they do not fit into one general purpose register but require two 
>> registers.
>> For classification purposes `__int128` is treated as if it were implemented
>> as:
>>
>>   typedef struct {
>>   long low, high;
>>   } __int128;
>>
>> with the exception that arguments of type `__int128` that are stored in
>> memory must be aligned on a 16-byte boundary
>
>
>
> - K1OM agrees 
> https://www.intel.com/content/dam/develop/external/us/en/documents/k1om-psabi-1-0.pdf
> - These types don't seem to be mentioned anywhere in i386 1997 
> https://www.sco.com/developers/devspecs/abi386-4.pdf
> - Also not in MIPS RISC 1996 
> https://math-atlas.sourceforge.net/devel/assembly/mipsabi32.pdf
> - MIPSpro64 doesn't mention 128-bit integers but does mention 128-bit floats. 
> From page 24 https://math-atlas.sourceforge.net/devel/assembly/mipsabi64.pdf
>
>> Quad-precision floating point parameters (C long double or Fortran REAL*16) 
>> are
>> always 16-byte aligned. This requires that they be passed in even-odd 
>> floating point
>> register pairs, even if doing so requires skipping a register parameter 
>> and/or a
>> 64-bit save area slot. [The 32-bit ABI does not consider long double 
>> parameters,
>> since they were not supported.]
>
>
>
> - From PPC64 section 3.1.4 
> https://math-atlas.sourceforge.net/devel/assembly/PPC-elf64abi-1.7.pdf:
>
>> [paraphrased from table]
>> type - sizeof - alignment
>> `__int128_t` - 16 - quadword
>> `__uint128_t` - 16 - quadword
>> `long double` - 16 - quadword
>
>
>
> - z/Arch: this is the only target that clang seems to align to 8, see [1]. 
> Also from 1.1.2.4 in https://github.com/IBM/s390x-abi/releases/tag/v1.6:
>
>> [paraphrased from table]
>> type - size (bytes) - alignment
>> `__int128` - 16 - 8
>> signed `__int128` - 16 - 8
>> `long double` - 16 - 8
>
> [1]: https://reviews.llvm.org/D130900




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

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

[Trevor Gross via Phabricator via cfe-commits]

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 nothing else supports i128.
> For x86, the current fp128 handling is incompatible. The use of i128 with 
> lower alignment in a call into libgcc breaks compatibility.
>
> For x64, the current i128 handling is compatible but fragile. The alignment 
> to 8 byte boundaries causes no compatibility issue because all calls into 
> libgcc pass values in registers. If support for _ _udivmodti4 and _ 
> _udivmodti4 were to be added in the future, the current i128 handling would 
> be wrong.
> For x64, the current fp128 handling is compatible. The alignment to 8 byte 
> boundaries causes no compatibility issue because all calls into libgcc pass 
> values in registers. No other libgcc functions use pointers.

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.

> ISSUES
>
> As far as I can tell, the compatibility issues in the current version of LLVM 
> are: the fp128 handling in x86, potentially the i128 handling in x64, and the 
> i128 handling in Rust.

Rust just uses LLVM's `i128` value directly so it doesn't necessarily need to 
be called out on its own (think we are in agreement here, just clarifying)

> [...]
> QUESTIONS
>
> Is the behaviour of LLVM, clang, GCC, and MSVC indeed as I described, or did 
> I make a mistake anywhere?

I believe that MSVC is in general ambiguous about these details on types that 
it does not support, but I would assume that being consistent with the Linux 
ABI is preferred and probably what MSVC would choose if they ever do decide on 
a specification for this type (unless LLVM has contact with Microsoft that may 
be able to clarify? They make no guarantees against breaking things in any 
case.)

> [...]

It probably makes sense to have reasoning for choosing the selected behavior 
and having something specific to test against, so I'll link what I know.

- From AMD4 ABI Draft 0.99.7 (2014) 
:

> [paraphrased from Figure 3.1]
> type - sizeof - alignment - AMD64 architecture
> long - 8 - 8 - signed eightbyte [I included this in the table for the below 
> reference]
> `__int128` - 16 - 16 - signed sixteenbyte
> signed `__int128` - 16 - 16 - signed sixteenbyte
> `long double` - 16 - 16 - 80-bit extended (IEEE-754)
> `__float128` - 16 - 16 - 128-bit extended (IEEE-754)
> [...]
> The `__int128` type is stored in little-endian order in memory, i.e., the 64
> low-order bits are stored at a a lower address than the 64 high-order bits
> [...]
> Arguments of type `__int128` offer the same operations as INTEGERs,
> yet they do not fit into one general purpose register but require two 
> registers.
> For classification purposes `__int128` is treated as if it were implemented
> as:
>
>   typedef struct {
>   long low, high;
>   } __int128;
>
> with the exception that arguments of type `__int128` that are stored in
> memory must be aligned on a 16-byte boundary



- K1OM agrees 
https://www.intel.com/content/dam/develop/external/us/en/documents/k1om-psabi-1-0.pdf
- These types don't seem to be mentioned anywhere in i386 1997 
https://www.sco.com/developers/devspecs/abi386-4.pdf
- Also not in MIPS RISC 1996 
https://math-atlas.sourceforge.net/devel/assembly/mipsabi32.pdf
- MIPSpro64 doesn't mention 128-bit integers but does mention 128-bit floats. 
From page 24 https://math-atlas.sourceforge.net/devel/assembly/mipsabi64.pdf

> Quad-precision floating point parameters (C long double or Fortran REAL*16) 
> are
> always 16-byte aligned. This requires that they be passed in even-odd 
> floating point
> register pairs, even if doing so requires skipping a register parameter 
> and/or a
> 64-bit save area slot. [The 32-bit ABI does not consider long double 
> parameters,
> since they were not supported.]



- From PPC64 section 3.1.4 
https://math-atlas.sourceforge.net/devel/assembly/PPC-elf64abi-1.7.pdf:

> [paraphrased from table]
> type - sizeof - alignment
> `__int128_t` - 16 - quadword
> `__uint128_t` - 16 - quadword
> `long double` - 16 - quadword



- z/Arch: this is the only target that clang seems to align to 8, see [1]. Also 
from 1.1.2.4 in https://github.com/IBM/s390x-abi/releases/tag/v1.6:

> [paraphrased from table]
> type - size (bytes) - alignment
> `__int128` - 16 - 8
> signed `__int128` - 16 - 8
> `long double` - 16 - 8

[1]: https://reviews.llvm.org/D130900


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