Page MenuHomePhabricator

[PowerPC] Enable OpenMP for powerpcle target. [5/5]
ClosedPublic

Authored by Bdragon28 on Dec 1 2020, 6:45 PM.

Details

Summary

Enable OpenMP for powerpcle to match the rest of powerpc*.

Update tests.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a reviewer: alexshap. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Bdragon28 requested review of this revision.Dec 1 2020, 6:45 PM

On FreeBSD, the main use of this will be on the new powerpc64le arch, where we need to build a 32-bit LE bootloader for use with pseries. (it is easier to retarget LLVM than make a cross-endian bootloader, as it would involve rewriting filesystem code etc.)

On Linux, I suspect the primary use will be with user-space CPU emulation tools that do syscall translation from foreign LE architectures.

On Linux, I set the defaults assuming that this target would primarily be used as a multiarch instead of the "native" arch for an install (i.e. assume lib32 instead of lib). If there are any existing powerpcle userlands out there that do not do this, it can be changed.

Note: I do not have commit access yet.

This seems problematic to me for a few reasons:

  1. There is no 32-bit toolchains or libraries for little endian Linux systems
  2. There is no support in the ELFv2 ABI for 32-bit object mode and there may be a number of places we assume that little endian systems use ELFv2 ABI
  3. There are a number of places that make the assumption that little endian systems must be 64-bit so there's no checking for 32-bit mode (which will likely result in 64-bit specific code gen)

Since this seems to be for a very specific use case, can we perhaps severely restrict the support for 32-bit little endian mode (perhaps BSD only, no Altivec/VSX, etc.)?

This patch should be split. I suggest that you create 4 patches.

  • llvm: triple change
  • llvm: llvm/Object/ELFObjectFile.h llvm-readobj llvm-objdump
  • clang
  • lld

This seems problematic to me for a few reasons:

  1. There is no 32-bit toolchains or libraries for little endian Linux systems
  2. There is no support in the ELFv2 ABI for 32-bit object mode and there may be a number of places we assume that little endian systems use ELFv2 ABI
  3. There are a number of places that make the assumption that little endian systems must be 64-bit so there's no checking for 32-bit mode (which will likely result in 64-bit specific code gen)

Since this seems to be for a very specific use case, can we perhaps severely restrict the support for 32-bit little endian mode (perhaps BSD only, no Altivec/VSX, etc.)?

Technically, 32 bit PowerPC systems are neither ELFv1 nor ELFv2, they are Power Architecture 32-bit ABI (which has various extensions) that does not have the complexities that plagued ELFv1 and is a relatively "normal" ABI.

I have not seen any places that this is problematic that I am not already addressing with this patch.

This patch should be split. I suggest that you create 4 patches.

  • llvm: triple change
  • llvm: llvm/Object/ELFObjectFile.h llvm-readobj llvm-objdump
  • clang
  • lld

Can do.

This seems problematic to me for a few reasons:

  1. There is no 32-bit toolchains or libraries for little endian Linux systems
  2. There is no support in the ELFv2 ABI for 32-bit object mode and there may be a number of places we assume that little endian systems use ELFv2 ABI
  3. There are a number of places that make the assumption that little endian systems must be 64-bit so there's no checking for 32-bit mode (which will likely result in 64-bit specific code gen)

Since this seems to be for a very specific use case, can we perhaps severely restrict the support for 32-bit little endian mode (perhaps BSD only, no Altivec/VSX, etc.)?

Technically, 32 bit PowerPC systems are neither ELFv1 nor ELFv2, they are Power Architecture 32-bit ABI (which has various extensions) that does not have the complexities that plagued ELFv1 and is a relatively "normal" ABI.

I have not seen any places that this is problematic that I am not already addressing with this patch.

I would be fine with restricting this to FreeBSD and/or limiting it to freestanding. I mainly added the Linux support as there was recent interest in 32-bit LE and it is a possibility there will be some experimental userlands at some point in the future. https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-May/211669.html

Bdragon28 added a comment.EditedDec 1 2020, 8:57 PM

Regarding things like Altivec and VSX:
Altivec should be just fine to run in 32-bit LE.
I am undecided as to whether VSX should be banned or not. However that goes, it should be identical in powerpc64 -m32 and powerpc64le -m32.

Regarding codegen:
CPU capabilities are a property of the selected CPU, not whether it is 32-bit/64-bit code, or LE/BE. (With the exception of certain instructions that are banned in LE, and certain instructions that are banned in 32-bit IIRC)

Regarding Book-III support for powerpcle:
I see zero value in using powerpcle for any Book-III activities. Modern LE kernels will always be 64-bit, and oldschool LE kernels were A) proprietary, and B) ancient history.
Since bridge mode is gone on modern TRUELE processors, it's pointless anyway, as you can't run a 32-bit kernel in either endian.

Regarding cpu support for powerpcle:
I see zero value in supporting non-TRUELE processors such as PPC74xx. Limiting it to POWER7 and above (or even POWER8 and above) seems sane to me. That is, I think it is a valid assumption
that powerpcle will not be being used in a "true" 32-bit environment where the 64-bit options are not implemented on the processor.

Regarding hosted use:
TBD. We don't have plans in FreeBSD for building a powerpcle compat32 userland. Linux userlands are experimental. There may be some open ABI questions. I'm fine with putting that off for now.

On FreeBSD, the main use of this will be on the new powerpc64le arch, where we need to build a 32-bit LE bootloader for use with pseries. (it is easier to retarget LLVM than make a cross-endian bootloader, as it would involve rewriting filesystem code etc.)

Excuse my ignorance, but what are there technical limitations preventing writing n 64-bit LE boot loader and avoid having a 32-bit LE target all-together?

Could you add some more information to the commit message to better describe the powerpcle target. It's not clear from the target name exactly what it is for.

On FreeBSD, the main use of this will be on the new powerpc64le arch, where we need to build a 32-bit LE bootloader for use with pseries. (it is easier to retarget LLVM than make a cross-endian bootloader, as it would involve rewriting filesystem code etc.)

Excuse my ignorance, but what are there technical limitations preventing writing n 64-bit LE boot loader and avoid having a 32-bit LE target all-together?

LoPAPR client binding requirements, section B.10.
"OF Client Programs for an LoPAPR platform shall execute in 32-bit mode with an OF cell size of 1."

FreeBSD loader on pseries is an OF client, and as such, while it is free to be either BE or LE (OF is required to adapt itself), it MUST be a 32-bit image.

q66 added a subscriber: q66.Dec 15 2020, 11:29 PM

I am interested in this, since we are currently bootstrapping an entire ppcle userland in our distribution (glibc and musl) - we already have everything generally working (on ppc64le host), with llvm being a notable blocker for some things - so i will be testing this in near future

q66 added a comment.Dec 15 2020, 11:33 PM

we are also interested in running this on "true" 32-bit hardware eventually, so not restricting to POWER7 would be a good thing, potentially

q66 added a comment.Dec 16 2020, 8:06 PM

I've tested the patch (applied to LLVM11) and can confirm it works. There have been some changes/fixes I needed to do, which I already reported on IRC, so I will not include them here (Bdragon28 should just be able to apply them and push them out)

Right now, I have a full ppcle Linux userland (which can fully compile itself, so it's self-hosted), I will be making binary packages available soon (source is already available through the Void Linux project) so anyone with a ppc64le kernel with a couple of fixes (also available through Void) should be able to run a full 32-bit LE environment

MaskRay added inline comments.Dec 16 2020, 10:32 PM
clang/lib/Basic/Targets/PPC.h
358 ↗(On Diff #308833)

redundant ()

clang/lib/CodeGen/CodeGenModule.cpp
971 ↗(On Diff #308833)

Drop this change.

The ppc32 code path was actually dead and I have deleted it about 2 weeks ago.

clang/lib/Driver/ToolChains/Linux.cpp
146 ↗(On Diff #308833)

Should it be powerpcle?

Bdragon28 updated this revision to Diff 313649.Dec 23 2020, 8:36 PM
Bdragon28 edited the summary of this revision. (Show Details)

Splitting up into multiple commits as per MaskRay's suggestion.

The lld/ELF part change looks good.

Bdragon28 updated this revision to Diff 313651.Dec 23 2020, 8:42 PM

Trying again..

Bdragon28 updated this revision to Diff 313654.Dec 23 2020, 9:27 PM
  • Address review comment from MaskRay.
  • Incorporate changes from the Void powerpcle patchset.
Bdragon28 marked 2 inline comments as done.Dec 23 2020, 9:33 PM
Bdragon28 added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
2152 ↗(On Diff #313654)

Pointed out by q66 in irc:

This triples list is bogus, and I need to update the comment too.

Bdragon28 updated this revision to Diff 313655.Dec 23 2020, 9:44 PM

Fix merge base.

Bdragon28 updated this revision to Diff 313930.Mon, Dec 28, 8:12 PM
  • Fix bug in clang/test/Driver/linux-header-search.cpp -- The powerpc64le test was being done with -m32 accidentally.
  • Update llvm/unittests/ADT/TripleTest.cpp for powerpcle.
  • Update gcc driver bits to reflect use of powerpcle in void-ppc.
Bdragon28 updated this revision to Diff 313931.Mon, Dec 28, 8:17 PM
  • Fix LLVM object handling unit test.

Add q66 to reviewers list for the targeting bits relevant to Void ppcle.

Bdragon28 updated this revision to Diff 314012.Tue, Dec 29, 1:55 PM
Bdragon28 retitled this revision from [PowerPC] Add powerpcle target. to [PowerPC] Add powerpcle target. (5/5).
Bdragon28 updated this revision to Diff 314019.Tue, Dec 29, 2:12 PM

Re-uploading patch for part 5 now that I have the dependency tree fixed.

Bdragon28 updated this revision to Diff 314052.Tue, Dec 29, 6:57 PM

Add missing OpenMP TLS test for powerpcle.

MaskRay accepted this revision.Tue, Dec 29, 7:27 PM

Thanks!

This revision is now accepted and ready to land.Tue, Dec 29, 7:27 PM
Bdragon28 retitled this revision from [PowerPC] Add powerpcle target. (5/5) to [PowerPC] Enable OpenMP for powerpcle target. [5/5].Sat, Jan 2, 9:00 AM
Bdragon28 edited the summary of this revision. (Show Details)
Bdragon28 updated this revision to Diff 314247.Sat, Jan 2, 9:19 AM

Forcing retest again.

This revision was automatically updated to reflect the committed changes.