This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Enable int128 in 64 bit mode
ClosedPublic

Authored by jsji on Oct 4 2021, 10:47 AM.

Details

Summary

This patch remove the override in AIX target,
so the int128 is enabled in 64 bit mode or with ForceEnableInt128.

Diff Detail

Event Timeline

jsji requested review of this revision.Oct 4 2021, 10:47 AM
jsji created this revision.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 4 2021, 10:47 AM

Making a change to whether a platform has __int128 affects the ABI of libc++'s std::chrono::file_clock. We should make sure the mitigation patch is posted and accepted.

llvm/test/CodeGen/PowerPC/int128_ldst.ll
31

Confirming that this case matches GCC on AIX:

ld 4,8(3)
ld 3,0(3)
blr

Type alignment (16), choice of GPRs for argument passing (first available including r10), va_arg invocations, order of components (big endian), and calls to double/float conversions all check out. It may be reasonable to locate existing tests for these and make sure they also test 64-bit AIX.

jsji added inline comments.Oct 5 2021, 7:20 AM
llvm/test/CodeGen/PowerPC/int128_ldst.ll
31

$ gcc -maix64 -S -O p128.c -o t.s; grep blr -B3 t.s
.check1:

ld 4,8(3)
ld 3,0(3)
blr

Making a change to whether a platform has __int128 affects the ABI of libc++'s std::chrono::file_clock. We should make sure the mitigation patch is posted and accepted.

Good point, but the std::chrono::file_clock interface hasn't shipped yet on the platform libc++, since it's currently still at a bit of a backlevel, so I don't think that should be blocking to enabling the type.

Good point, but the std::chrono::file_clock interface hasn't shipped yet on the platform libc++, since it's currently still at a bit of a backlevel, so I don't think that should be blocking to enabling the type.

I am concerned that __int128 is an over-aligned type on AIX. The required alignment of the type is greater than the greatest fundamental alignment supported by default on the platform.

jsji updated this revision to Diff 377283.Oct 5 2021, 9:43 AM

Address comments -- adding AIX triples to more existing tests.

jsji added a comment.Oct 12 2021, 6:52 AM

Ping @hubert.reinterpretcast Any further comments?

lkail added a comment.EditedOct 12 2021, 10:33 PM

I would like to see aix triple added to more existing tests involving ABI in llvm/test/CodeGen/PowerPC(such as ppc64-i128-abi.ll), since AIX has independent calling convention lowering and is different from linux.

jsji updated this revision to Diff 379485.Oct 13 2021, 11:53 AM

Add AIX XCOFF triples to i128 tests, especially ppc64-i128-abi.ll.

jsji added a comment.Oct 13 2021, 12:35 PM

ppc64-i128-abi.ll CHECK-P9 part depends on D94282: [PowerPC] Support ppc-asm-full-reg-names for AIX.

lkail accepted this revision as: lkail.Oct 13 2021, 10:39 PM

This LGTM as the start point to support int128 on AIX. We might need more patches involving libraries in the LLVM monorepo, we can do that progressively.

This revision is now accepted and ready to land.Oct 13 2021, 10:39 PM

This LGTM as the start point to support int128 on AIX. We might need more patches involving libraries in the LLVM monorepo, we can do that progressively.

Agreed (although I find it slightly odd that the testing doesn't check the edge case where the argument value is split between r10 and the stack).

jsji added a comment.Oct 15 2021, 8:29 AM

This LGTM as the start point to support int128 on AIX. We might need more patches involving libraries in the LLVM monorepo, we can do that progressively.

Agreed (although I find it slightly odd that the testing doesn't check the edge case where the argument value is split between r10 and the stack).

I can add more test in a follow up patch.

This revision was landed with ongoing or failed builds.Oct 15 2021, 9:23 AM
This revision was automatically updated to reflect the committed changes.