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

A small step toward building a toolchain on M1 Mac #3500

Closed
wants to merge 3 commits into from
Closed

A small step toward building a toolchain on M1 Mac #3500

wants to merge 3 commits into from

Conversation

johnno1962
Copy link
Contributor

Minor change removing impediment to building Swift toolchain on M1 Mac

Motivation:

Would like to be able to work on the Swift compiler on an M1 Mac.

Modifications:

Very small change for when swift/utils/build-toolchain is run on M1 Mac.

Result:

Will not be prevented from building toolchain on M1 Mac.

@@ -805,7 +805,7 @@ def get_swiftpm_flags(args):

build_target = get_build_target(args)
cross_compile_hosts = args.cross_compile_hosts
if build_target == 'x86_64-apple-macosx' and "macosx-arm64" in cross_compile_hosts:
if (build_target == 'x86_64-apple-macosx' or build_target == 'arm64-apple-macosx') and "macosx-arm64" in cross_compile_hosts:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't do what you think it does: it will have no effect on building SPM for an M1 mac.

What this does is that if the host is an M1 mac and you somehow pass in macosx-arm64 as a cross-compilation host, it will build SPM for x86_64 too, which makes no sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure of the specifics myself but this change made the difference between being able to build a toolchain using swift/utils/build-toolchain and receiving the error a couple of lines down.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's quite unfortunate that the cross-compilation here is so asymmetrical. As @johnno1962 points out it isn't currently possible to build a universal toolchain on M1 because of the existing check for the host architecture, and this change does seem to fix that, though the logic looks as if it doesn't make any sense. I think this is because the logic that builds the toolchain passes -cross-compile-hosts macos-arm64 to mean "build a universal toolchain for x86_64 and arm64", which is what doesn't make sense to begin with.

@shahmishal if this were cleaned up to essentially say --build-universal and imply the same semantics, could the scripts that invoke this logic as part of the toolchain build be fixed? It would be good to clean this up now that more people are using M1 Macs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see what you mean now: the issue is that the base mac preset assumes that it is building on an Intel Mac and passes arm64 as a cross-compilation host. This change is merely a hack to make that work, you should be fixing those presets instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up. I've updated the ticket documenting my attempts to build a toolchain and the various "hacks" I've had to apply here: https://bugs.swift.org/browse/SR-14035.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a universal flag would work and yes, this is the only preset flag that implies building on an Intel mac, but I think it was a mistake to build universal by default to begin with, as I don't think most people want both an Intel and arm64 mac toolchain to begin with. That base preset used by build-toolchain should only build for the host, as it always did until this flag was added last year.

I think Mishal just added it for the CI, to test cross-compiling the toolchain for arm64 too, but unfortunately that is also the command we prescribe for devs building the toolchain, sticking them with an interminable build of both toolchains when they will only want to build for the host.

@johnno1962, try commenting out that flag I linked a couple days ago and only building the toolchain for arm64, without this patch. If that works fine, you should submit a pull for the main Swift repo changing the presets somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a new commit removing the filtering by build_target altogether as an alternative as it seems to serve no purpose and build_target seems like a misnomer anyway. I'd rather not tangle with the main Swift repo to "change the presets somehow" as the port of building toolchains on arm64 really needs to be tackled as a whole by someone who has "the big picture". I've performed a preliminary analysis in the Jira ticket and it seems like there are only 4 relatively minor problems which someone with knowledge of cmake & llvm should make short work of. I agree though it's regrettable the build-toolchain script builds universal by default but building the package manager universal won't extend toolchain build times by much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @buttaface and @johnno1962 — it's good that it no longer depends on what the host machine is, but it's slightly unfortunate that passing macosx-arm64 on a M1 machine still results in a universal build whereas passing macosx-x86_64 doesn't. However, I think that the fact that --cross-compile-hosts is being passed at all is a strong sign that the caller wants a universal toolchain, so it almost doesn't matter what gets passed as a parameter there. Perhaps the presence of the flag at all should be considered a boolean, i.e. if there's any value at all passed for that parameter, then you get a universal build. That's the only further change I would suggest here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @abertelrud, I've pushed a commit that only checks for the presence of cross_compile_hosts which (even if it reads as a terrible piece of English) I agree is probably the safest option at this stage. The "else" side of the "if" doesn't quite make sense now but this small section of code can be revisited if there is ever a time when more than two architectures need to be supported.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @johnno1962. Based on @buttaface's comment below it seems that might have provided poor advice here, in light of PR #3403 that is up to add Android support here. I will read that in more detail in reply below here.

@abertelrud
Copy link
Contributor

@swift-ci please smoke test

@@ -805,7 +805,7 @@ def get_swiftpm_flags(args):

build_target = get_build_target(args)
cross_compile_hosts = args.cross_compile_hosts
if build_target == 'x86_64-apple-macosx' and "macosx-arm64" in cross_compile_hosts:
if cross_compile_hosts is not None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not going to work when other cross-compilation targets are added, such as in my Android pull #3403. As I said, this is a hack that should not be merged: the real fix is upstream in the Swift compiler repo where this flag is incorrectly passed in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. I wonder if we're conflating two concepts here: compile for a different platform vs. compile for all the architectures for a platform. For example, in general it's not going to be possible (I believe) to build for more than one platform at a time, while it's possible to build for as many architectures as that one platform will support.

When you say that the Swift compiler repo is incorrectly passing this flag: would it work to have a different flag that causes all the architectures to be built? It seems to me that that's what's needed when building toolchains on a particular platform, whether it's Darwin or something else: the distributable builds should have all the architectures supported by that platform (whereas debug builds might not, for performance reasons).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me begin by saying that this line that is being replaced was a hack too, as it was added just a month before the 5.3 release in 6306663 then merged back to master in 242e733. It only cross-compiles one-way, from macOS Intel to arm64 as you noticed too, which made sense as there wasn't much Apple Silicon (AS) hardware at the time. It will need to be cleaned up now that there is more AS hardware out there, but this pull is the wrong way to do it.

I suggest that one of you talk to Mishal and find out if he really wants build-toolchain to build both toolchains by default, as that is the command prescribed to even new Swift devs. I suspect he did not intend that and just meant to test this cross-compilation on the CI. Depending on what he says, you can work out how best to rework this. Note that if you really know what you're doing and use build-script instead, you will not hit this issue: this is only a problem with that preset and build-toolchain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the additional observations @buttaface — I didn't realize that the desktop build instructions for Swift involves a universal build. That seems suboptimal.

@shahmishal Can we fix this so that:

  • build-toolchain (or whatever script we tell people to use on the desktop) only builds the host architecture by default
  • the script that builds the official toolchain passes some option to build it universal
  • we change the SwiftPM bootstrap script to let it be built universally regardless of what architecture it's currently being built on

I can take care of the SwiftPM changes but it sounds as if there are other changes needed to the Swift build scripts, and I'm not at all familiar with them. What other things need to happen to fix this?

What I'd like to end up with from SwiftPM's perspective is that running bootstrap with no options should only build for the host (as it does today), but that there should be a single standard option (e.g. --build-universal) that builds for all the architectures (currently x86_64 and arm64 on Darwin, but possibly other combinations on other platforms) regardless of the host architecture.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build-toolchain (or whatever script we tell people to use on the desktop) only builds the host architecture by default

We should add a flag to build-toolchain to all for user to build universal or host specific toolchain.

the script that builds the official toolchain passes some option to build it universal

Agree, we should continue to build universal for nightly and release toolchains.

we change the SwiftPM bootstrap script to let it be built universally regardless of what architecture it's currently being built on

This should be based on the argument provided by build-script to avoid building missing arch in swiftpm. I wonder if we can look up which arch swift was built with and build matching arch. (lipo -info)

@finagolfin
Copy link
Contributor

@johnno1962, could you change this to match what swift-driver does for now and we'll get this in?

@johnno1962
Copy link
Contributor Author

Hi @buttaface. Sorry, but as this was outstanding for while I deleted my swifpm fork and am unable to update or test it properly. Could I leave this to you please?

@finagolfin
Copy link
Contributor

Alright, just close this pull then, we'll get a fix in along the lines sketched out by Anders and Mishal above eventually.

@johnno1962
Copy link
Contributor Author

OK, cheers, Closing now. Do you know if there is any progress on being able to work in the compiler on an M1 (the ticket I mentioned)?

@johnno1962 johnno1962 closed this Jun 28, 2021
@finagolfin
Copy link
Contributor

That ticket says you got it working though, right? No idea if anyone is working on upstreaming the remaining fixes for M1.

@johnno1962
Copy link
Contributor Author

johnno1962 commented Jun 28, 2021

I hacked it until building a toolchain ran to completion more like. The resulting toolchain had it's issues but it was enough for my purposes in pursuing this PR. It really needs someone with clang and cmake knowledge to come up with the right solution. I'm baffled I seem to be the only person trying to get it working, M1s are sooo much faster.

@finagolfin
Copy link
Contributor

No, there were others who worked on the compiler portions before you, which is why you only hit problems in later parts of the toolchain build, which they may not have bothered with. I suggest you confer with them and you all get your patches together and try to upstream them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants