Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Add architecture PPC64le to Architecture enum #67428

Closed
Sapana-Khemkar opened this issue Apr 1, 2022 · 41 comments · Fixed by #68809
Closed

[API Proposal]: Add architecture PPC64le to Architecture enum #67428

Sapana-Khemkar opened this issue Apr 1, 2022 · 41 comments · Fixed by #68809
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Milestone

Comments

@Sapana-Khemkar
Copy link
Contributor

Sapana-Khemkar commented Apr 1, 2022

Background and motivation

All architectures, whether officially Microsoft supported or not, need to exist within the System.Runtime.InteropServices.Architecture enum. We want to introduce a new community-supported architecture ppc64le.
The implementation PR is #68809

API Proposal

namespace System.Runtime.InteropServices
{
    public enum Architecture
    {
        X86 = 0,
        X64 = 1,
        Arm = 2,
        Arm64 = 3,
        Wasm = 4,
        S390x = 5,
        LoongArch64 = 6,
        Armv6 = 7,
+       PPC64le = 8,
    }
}

API Usage

 case Architecture.PPC64le:
    Assert.Equal(Architecture.PPC64le, processArch);
    break;

Alternative Designs

No response

Risks

No response

@Sapana-Khemkar Sapana-Khemkar added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 1, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 1, 2022
@ghost
Copy link

ghost commented Apr 1, 2022

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

All architectures, whether officially Microsoft supported or not, need to exist within the System.Runtime.InteropServices.Architecture enum. We want to introduce a new community-supported architecture ppc64le.
The implementation PR is #67378

API Proposal

namespace System.Runtime.InteropServices
{
    public enum Architecture
    {
        X86 = 0,
        X64 = 1,
        Arm = 2,
        Arm64 = 3,
        Wasm = 4,
        S390x = 5,
        LoongArch64 = 6,
        Armv6 = 7,
+     POWERPC64 = 8,

    }
}

API Usage

 case Architecture.POWERPC64:
                    Assert.Equal(Architecture.POWERPC64, processArch);
                     break;

Alternative Designs

No response

Risks

No response

Author: Sapana-Khemkar
Assignees: -
Labels:

api-suggestion, area-System.Runtime.InteropServices, untriaged

Milestone: -

@MichalStrehovsky
Copy link
Member

Why all caps? The architecture seems to be referred to as PowerPC everywhere, not POWERPC (e.g. wikipedia: https://en.wikipedia.org/wiki/PowerPC).

@Sapana-Khemkar
Copy link
Contributor Author

Why all caps? The architecture seems to be referred to as PowerPC everywhere, not POWERPC (e.g. wikipedia: https://en.wikipedia.org/wiki/PowerPC).

Agree. Changed

@AaronRobinsonMSFT

This comment was marked as outdated.

@tannergooding
Copy link
Member

Should this be PowerPC or PowerPCFP (or some variation that indicates PowerPC + Floating Point support). They are considered different by the PE file format: https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#machine-types

@akoeplinger
Copy link
Member

Or PowerPC64LE to make it clear it's the little endian version.

@MichalStrehovsky
Copy link
Member

Should this be PowerPC or PowerPCFP (or some variation that indicates PowerPC + Floating Point support). They are considered different by the PE file format: https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#machine-types

We don't represent float support in Architecture.Arm - what makes PowerPC64 special in this respect?

Or PowerPC64LE to make it clear it's the little endian version.

We don't represent endianness in Architecture.Arm - what makes PowerPC64 special in this respect?

@tannergooding
Copy link
Member

We don't represent float support in Architecture.Arm - what makes PowerPC64 special in this respect?

Other architectures/ABIs are differentiating, including the PE file format. That may or may not be important when considering the Architecture enum. It's a consideration that should be made.

@Sapana-Khemkar Sapana-Khemkar changed the title [API Proposal]: Add architecture POWERPC64 to Architecture enum [API Proposal]: Add architecture PowerPC64 to Architecture enum Apr 4, 2022
@akoeplinger
Copy link
Member

We don't represent endianness in Architecture.Arm - what makes PowerPC64 special in this respect?

While Arm technically can run in big-endian mode, practically all implementations and ports are for little-endian (e.g. there's no big-endian Linux support) so the distinction is less important.

PowerPC on the other hand shipped as primarily big-endian for much of its lifetime while the little-endian ppc64le is a relatively recent addition which I think might warrant the distinction in the enum name.

@Sapana-Khemkar
Copy link
Contributor Author

As per my understanding LE is not required as we can check using IsLittleEndian. Also we are supporting it only on Linux which is LE only. Also found similar discussion here

@BillSeurer
Copy link
Contributor

powerpc64 (sometimes ppc64) is used by other open source stuff for big endian.
powerpc64le (sometimes ppc64le) is used for little endian.

And BE is supported on Linux. I use powerpc64 BE Linux servers all the time.

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Apr 4, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the Future milestone Apr 4, 2022
@AaronRobinsonMSFT
Copy link
Member

Labeling this as "Future" is not meant to indicate it isn't important but more bookkeeping on our side. As soon as all stakeholders agree on what should be the correct name, we can flip this to "ready for review".

@janani66
Copy link

janani66 commented Apr 4, 2022

powerpc64 (sometimes ppc64) is used by other open source stuff for big endian. powerpc64le (sometimes ppc64le) is used for little endian.

And BE is supported on Linux. I use powerpc64 BE Linux servers all the time.

Agreed. We should use the label PowerPC64le. to make it clear that this is for the little endian powerpc64 platform.

@Sapana-Khemkar
Copy link
Contributor Author

Sapana-Khemkar commented Apr 4, 2022 via email

@Sapana-Khemkar
Copy link
Contributor Author

Are we all okay with enum name PowerPC64le?

@Sapana-Khemkar Sapana-Khemkar changed the title [API Proposal]: Add architecture PowerPC64 to Architecture enum [API Proposal]: Add architecture PowerPC64le to Architecture enum Apr 6, 2022
@MichalStrehovsky
Copy link
Member

I'm not the decision maker but I still don't see the reason to include endianness. We don't include it for ARM. We also don't include float mode on ARM (even though Linux does distinguish soft and hard float modes when referring to the ISA and we do support both on the VM side).

This enum has so far represented the ISA, not the binary interface (see also #26634 (comment)). Adding the endianness doesn't fit with the rest.

@Sapana-Khemkar
Copy link
Contributor Author

If I change Architecture enum to PowerPC64 then System.Runtime.InteropServices.RuntimeInformationTests.RuntimeIdentifierTests.VerifyOSRid testcase fails.
This test case compares Architecture enum value and the architecture component of the RID. On Power machine Linux reported architecture name is ppc64le.
So if we go as per this test case we have to keep enum name as PPC64LE or Ppc64le. Otherwise test case needs to be changed.

@BillSeurer
Copy link
Contributor

Do the other architectures match the linux reported architecture names? In any case ppc64le should work fine.

@BillSeurer
Copy link
Contributor

For what it is worth I checked on my mac and the architecture there is reported as "i386" and on my old Linux thinkpad it is reported as "x86_64" neither of which seem to match the X86 in the enum list.

@uweigand
Copy link
Contributor

I'm not the decision maker but I still don't see the reason to include endianness. We don't include it for ARM. We also don't include float mode on ARM (even though Linux does distinguish soft and hard float modes when referring to the ISA and we do support both on the VM side).

This enum has so far represented the ISA, not the binary interface (see also #26634 (comment)). Adding the endianness doesn't fit with the rest.

My understanding is the following. Please correct me if any of these assumptions is wrong:

  • The Architecture enum value should match the architecture component of the RID (modulo upper/lower casing). This is in fact verified by the test mentioned here [API Proposal]: Add architecture PPC64le to Architecture enum #67428 (comment)
  • The RID identifies the architecture of a package, in particular of native components within a package; if the package RID matches the platform RID, native components in the package are supposed to run on the platform

If that is true, it follows that we would need two different RID values for big- and little-endian PowerPC, and therefore two different Architecture enum values, because BE and LE PowerPC are different platforms that are not binary compatible. (This is not only due to byte order, they also use different ABIs.)

The next question is whether to use ppc64(le) or powerpc64(le). Unfortunately, there is precedent for both: the Linux kernel uses ppc64(le) as architecture identifier and reports that from uname -m. However, the GNU target triple, used with GCC and LLVM as well as in configure scripts of many open source packages, uses powerpc64(le) as architecture identifier.

This actually is very similar to the situation on 64-bit Arm, where the Linux kernel uses arm64 but the GNU target triple uses aarch64. Here, .NET seems to have made the choice to follow the kernel and use arm64 for the RID and the architecture enum. Following that precedent would argue for using ppc64 and ppc64le for our platform.

The final question would be about upper/lower casing of the Architecture enum (the RID is all lower case anyway). Coding standards seem to require that enum values start with an upper case letter, so some of the options would be Ppc64le, PPC64le or PPC64LE. Not sure what to prefer here, this seems a cosmetic choice.

@MichalStrehovsky
Copy link
Member

We use the armel RID for ARM-softfloat ABI and arm RID for ARM-hardfloat (or armhf in Linux lingo) (source). There's only one member of the Architecture enum for ARM, despite supporting two ABIs and having representation for them in the form of a RID.

I don't think Architecture represents ABI or has a relationship with the RID.

@MichalStrehovsky
Copy link
Member

Well, there is a test case explicitly verifying the relationship between Architecture and the RID

That test happens to work for the architectures we run tests on. Armel is community supported.

Armbe RID for big endian arm would be in the same boat if it's ever added.

@janani66
Copy link

What further information is needed to close on this issue? Since the Power BE and LE are completely incompatible like Ulrich mentioned, including the endianness in the name is preferred.

@gerrith3
Copy link

Throughout thousands of communities, we use the ppc64le tag to identify our architecture. There is the historical big endian architecture on linux which IBM doesn't support, although various communities still do, including the older mono. We also have AIX and IBM i people who have done the old mono and have expressed some interest in getting full .NET on those architectures which are big endian, along with a different API, different libraries, etc. They are about as much alike as Windows and Linux now. I would strongly recommend using a ppc64le variant for this.

@Sapana-Khemkar Sapana-Khemkar changed the title [API Proposal]: Add architecture PowerPC64le to Architecture enum [API Proposal]: Add architecture PPC64le to Architecture enum May 3, 2022
@teo-tsirpanis
Copy link
Contributor

@AaronRobinsonMSFT (or someone else), can this move forward? Adding community support for PPC64le is underway. You can decide on the name at the API review.

@directhex
Copy link
Member

directhex commented May 3, 2022

We should move forward with ppc64le as used in #68809.

  1. Architecture names on Linux are entirely cursed, and we're already largely ignoring them to do what we feel like. Dotnet's x64 architecture can also reasonably be referred to as x86_64, x86-64, amd64, AMD64, Intel 64, em64t, and probably more. We're not picking "the right value" for any of our architectures, we're picking "a value which is probably understandable to our users". "PPC" vs "Power" vs "PowerPC" are not confusing to users of the architecture, any more than using x86 is confusing to a user of a Linux distribution which calls that architecture i386.
  2. Architecture names matter from a "telling things apart" perspective. There are no existing cases in the runtime where we need to tell endianness apart because there is a risk of confusion (s390x is BE-only; big-endian ARM was never meaningfully A Thing; and the MIPS64 port seems dead although that is 100% a place where we could have this discussion given the prevalence of both variants in single-endian systems). Consider the RID graph - how is someone publishing a NuGet package with unmanaged components (say SkiaSharp or a Git library) supposed to support both big and little endian, if the directory is supposed to be the same because the RID is the same? They're 100% incompatible architectures, at an OS level.
  3. Big endian came first for Power, so the Power user community views architecture names like powerpc, ppc, powerpc64 or ppc64 as referring to big endian for historic reasons. The little endian suffix came much later. This is codified in, say, Fedora's architectures (ppc64 vs ppc64le). If we divert from this, we are introducing confusion. There is no advantage in us saying "we are right, and 30 years of this platform's users are wrong". It's just hubris.

tl;dr: Only names with endianness included are meaningful. Either ppc64le or powerpc64le are meaningful to the user community. The capitalization is an irrelevant detail and doesn't matter at all. And we already have a Docker image for building against which uses ppc64le throughout, since IBM seemed to favour this.

@AaronRobinsonMSFT AaronRobinsonMSFT added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 4, 2022
@YohanSciubukgian
Copy link

Could you also consider adding support for ppc 32 bits ?
Cf : #26634

@directhex
Copy link
Member

Could you also consider adding support for ppc 32 bits ? Cf : #26634

That's out of scope for this PR, we prefer to only add arches to the enum which are explicitly being used, and there's active work underway at IBM's end to support PPC64le. Until there's signs of life on a big-endian PPC32 port, it'd be pointless adding to this API change.

@huoyaoyuan
Copy link
Member

Show we mark this as blocking like other architecture name PR, since there's supporting PR ready?

@Sapana-Khemkar
Copy link
Contributor Author

@AaronRobinsonMSFT @jkoritzinsky can you please review this?

@AaronRobinsonMSFT AaronRobinsonMSFT modified the milestones: Future, 7.0.0 May 31, 2022
@AaronRobinsonMSFT
Copy link
Member

Will try to push on this to get it through API review ASAP.

@MichalStrehovsky
Copy link
Member

Will try to push on this to get it through API review ASAP.

API review board should spend some time discussing the purpose of this enum.

RID and Architecture are currently disconnected so if we want a 1:1 relationship with the RID like the comments are asking for (adding PowerPC64LE instead of PowerPC64), we might want to consider adding Architecture.Armel for Samsung-supported variants at the same time for parity.

The armel RID already exists (it's solving the problem with native assets @directhex refers to because it's the same issue), but we don't have an Architecture enum member for armel.

@Sapana-Khemkar
Copy link
Contributor Author

@AaronRobinsonMSFT @MichalStrehovsky any update on this. When API review board meets?

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Jun 7, 2022

@Sapana-Khemkar This is on the docket for me to rally support for again. Some questions that keep coming up are around RIDs - that particular angle is causing a fair bit of debate. Please ping me tomorrow if I don't respond here later today.

@teo-tsirpanis
Copy link
Contributor

@AaronRobinsonMSFT

@AaronRobinsonMSFT
Copy link
Member

Thanks @teo-tsirpanis, have not forgotten about this. I think we've reached a point where we can move forward. @directhex is going to help guide this through the API review. They will set the issue to blocking when it aligns with their availability.

@Sapana-Khemkar
Copy link
Contributor Author

Thanks @teo-tsirpanis, have not forgotten about this. I think we've reached a point where we can move forward. @directhex is going to help guide this through the API review. They will set the issue to blocking when it aligns with their availability.

Thanks @AaronRobinsonMSFT for update

@bartonjs
Copy link
Member

bartonjs commented Jun 9, 2022

  • We had a bit of a question of "is the -LE suffix necessary?" and it seems like most systems make a distinction between a ppc64 and a ppc64le, so "yes".
  • For naming consistency and .NET's naming guidelines, we changed the casing to only the initial P being capital (Ppc64le).
namespace System.Runtime.InteropServices
{
    public enum Architecture
    {
        X86 = 0,
        X64 = 1,
        Arm = 2,
        Arm64 = 3,
        Wasm = 4,
        S390x = 5,
        LoongArch64 = 6,
        Armv6 = 7,
+       Ppc64le = 8,
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 9, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 10, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 13, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Projects
None yet
Development

Successfully merging a pull request may close this issue.