Page MenuHomePhabricator

[compiler-rt][CMake] Enable TF intrinsics on powerpc32 Linux
ClosedPublic

Authored by raj.khem on Mar 10 2022, 7:46 AM.

Details

Summary

clang generates calls to these intrinsics when used for ppc32/linux, when using libgcc this works ok but when using compiler-rt for rtlib it fails with missing intrinsic symbols. also see [1]

[1] https://lists.llvm.org/pipermail/llvm-dev/2014-May/072784.html

Diff Detail

Event Timeline

raj.khem created this revision.Mar 10 2022, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 7:46 AM
raj.khem requested review of this revision.Mar 10 2022, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 7:47 AM
Herald added subscribers: Restricted Project, pcwang-thead. · View Herald Transcript
raj.khem edited the summary of this revision. (Show Details)Mar 10 2022, 7:52 AM

Note: powerpc64 defines __powerpc__ as well. You need to check whether powerpc64 needs this as well.

Note: powerpc64 defines __powerpc__ as well. You need to check whether powerpc64 needs this as well.

powerpc64 was covered under __LP64__ define previously, adding __powerpc__ wont change anything for ppc64

MaskRay added inline comments.Mar 10 2022, 2:20 PM
compiler-rt/lib/builtins/CMakeLists.txt
626

If ppc32 and ppc64 are going to share most files, perhaps rename this variable to powerpc_SOURCES.

raj.khem updated this revision to Diff 414562.Mar 10 2022, 7:17 PM
MaskRay accepted this revision.Mar 10 2022, 7:22 PM

LGTM.

This revision is now accepted and ready to land.Mar 10 2022, 7:22 PM

@MaskRay Any chance you could commit this change?

glaubitz accepted this revision.Jul 24 2022, 1:38 AM
This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.EditedAug 1 2022, 3:12 AM

This break the build of compiler-rt with GCC. Even recent GCC fails with "error: unable to emulate 'TI'" for 32-bit powerpc targets. The fact that this even works on Clang seems like a bug, because replacing it with typedef __int128 ti_int results in an __int128 is not supported on this target error in both GCC and Clang.

I think this change is incorrect, and trying to work around a codegen bug. We should not be emitting ti libcalls on 32-bit ppc targets, just like we don't do so for other 32-bit targets like i686 or armv7. I also wasn't able to actually reproduce the emission of such libcalls -- the linked mail (from 2014) refers to shift libcalls, but if I compile this code for powerpc-unknown-linux, I get an expanded instruction sequence (as expected):

typedef _BitInt(128) ti_int;

ti_int test(ti_int x, ti_int y) {
    return x >> y;
}

The only case I found that does emit a libcall is division, but that's an entirely different problem that is being solved by D126644 (it exists for all targets, e.g. 128 bit division will result in a missing __divti3 libcall on i686 as well).

glaubitz added a subscriber: nik.Aug 2 2022, 3:05 AM

@nik

This break the build of compiler-rt with GCC. Even recent GCC fails with "error: unable to emulate 'TI'" for 32-bit powerpc targets. The fact that this even works on Clang seems like a bug, because replacing it with typedef __int128 ti_int results in an __int128 is not supported on this target error in both GCC and Clang.

I guess we should revert this change then?

This break the build of compiler-rt with GCC. Even recent GCC fails with "error: unable to emulate 'TI'" for 32-bit powerpc targets. The fact that this even works on Clang seems like a bug, because replacing it with typedef __int128 ti_int results in an __int128 is not supported on this target error in both GCC and Clang.

I guess we should revert this change then?

I think so. I put up a revert patch in D130988.