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

llvm@7: add support for Apple Silicon #65628

Closed
wants to merge 1 commit into from
Closed

Conversation

hjelmn
Copy link
Contributor

@hjelmn hjelmn commented Nov 25, 2020

This commit updates the llvm@7 formula to allow it to be built on Apple
Silicon Macs. This package is needed to build ghc@8.8 on this platform.

The change includes backporting several patches from llvm master which
fix compilation and building on macOS with arm64.

Signed-off-by: Nathan Hjelm hjelmn@google.com

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

This commit updates the llvm@7 formula to allow it to be built on Apple
Silicon Macs. This package is needed to build ghc@8.8 on this platform.

The change includes backporting several patches from llvm master which
fix compilation and building on macOS with arm64.

Signed-off-by: Nathan Hjelm <hjelmn@google.com>
@BrewTestBot BrewTestBot added the legacy Relates to a versioned @ formula label Nov 25, 2020
Comment on lines +32 to +35
patch do
url "https://gist.githubusercontent.com/hjelmn/9fa1a7742acad71f404544a03bda414a/raw/b56a25f7208023d9724f671dfd7a80cbdac2a612/cfe-7.1.0-darwin-arm64.patch"
sha256 "eb6be925cdab150f5d01d51fa6bd6a4fe1dbf481794b83976ff877524192d1c1"
end
Copy link
Member

Choose a reason for hiding this comment

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

These are gonna need to be submitted upstream to be considered for inclusion in homebrew

Choose a reason for hiding this comment

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

LLVM 7 isn't updated anymore upstream, this is basically a legacy package.

Copy link
Member

Choose a reason for hiding this comment

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

If it's abandoned it should be deprecated and we still shouldn't use random patches that aren't submitted upstream

Copy link
Contributor Author

@hjelmn hjelmn Nov 25, 2020

Choose a reason for hiding this comment

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

These changes are all upstream. Just backported to llvm7 because it is no longer maintained. The only reason I request this is llvm7 is needed to build the ghc llvm backend. There is no native backend for ARM64 yet.

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 suppose the alternative is I make a keg, correct?

@fxcoudert
Copy link
Member

In Homebrew, the only thing that depends on llvm@7 is nvc, which has very low analytics and is not likely to get recent LLVM support anytime soon (nickg/nvc@c3d1ae5). So we don't have a need for an ARM backport, really.

This package is needed to build ghc@8.8 on this platform

Can you explain this? None of our ghc formulas need LLVM to build, they all build fine with system compiler.

@hjelmn
Copy link
Contributor Author

hjelmn commented Nov 25, 2020

Building ghc 8.8 (I need to try 8.10 but it likely has the same issue since there still is no native code generator) and it errors out with a newer llvm. Complains that the llvm is too new and says to use 7. From what I can tell this doesn't affect x86_64 because that uses a different backend. I can run some more attempts but the turn around is slow since ghc needs to be built twice. Once cross compiled and once native. I may just build a call with these until upstream ghc natively supports arm64.

@hjelmn
Copy link
Contributor Author

hjelmn commented Nov 26, 2020

This will take a couple of days but I will try to build with the latest llvm in homebrew. I will report back if there is an error (I saw one before with llvm 11). I don't really care about llvm@7 except as a dependency to ghc@8.8.

@hjelmn
Copy link
Contributor Author

hjelmn commented Nov 28, 2020

ghc 8.10.2 builds with llvm 11 (the formula in homebrew already works on Apple Silicon). ghc 8.10.2 requires patching to deal with issues with the aarch64 vs arm64 architecture naming but otherwise it seems to work fine. 8.8.4 mostly works but has some issues (after the same set of patches) with the linker code. these were fixed somewhere between 8.8.4 and 8.10.2.

@carlocab
Copy link
Member

Does the stable version of llvm 11 work? With libomp?

@fxcoudert
Copy link
Member

Does the stable version of llvm 11 work? With libomp?

Released llvm 11.0.0 builds and works on ARM Big Sur. We carry a small patch for libomp in the formula.

@hjelmn
Copy link
Contributor Author

hjelmn commented Nov 30, 2020

Both my ghc 8.8 and 8.10 formula changes fail because of the shim compiler adding linker flags when cross compiling:

[291 of 291] Compiling Main             ( utils/ghc-cabal/Main.hs, bootstrapping/Main.o )
Linking utils/ghc-cabal/dist/build/tmp/ghc-cabal ...
ld: in '/opt/homebrew/opt/llvm/lib/libunwind.dylib', building for macOS-x86_64 but attempting to link with file built for macOS-arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
`clang' failed in phase `Linker'. (Exit code: 1)
make[1]: *** [utils/ghc-cabal/dist/build/tmp/ghc-cabal] Error 1
make: *** [all] Error 2

I tried to bypass these wrappers by prepending /usr/bin to the path but they still get used. If there isn't a way to bypass the shim compilers then I will have to modify them to work when cross compiling in this way.

@carlocab
Copy link
Member

carlocab commented Nov 30, 2020

I've seen standard overrides for the shims by replacing/setting environment variables, though I imagine you've already tried that...

Also, --env=std flag to brew install? (I haven't used this, so maybe I'm confused about how this works.)

@hjelmn
Copy link
Contributor Author

hjelmn commented Nov 30, 2020

Looks like --env=std does indeed block the wrappers from getting in the way.

With --env=std --interactive:

bash-3.2$ type -p clang
/opt/homebrew/opt/llvm/bin/clang

Without:

bash-3.2$ type -p clang
/opt/homebrew/Library/Homebrew/shims/mac/super/clang

@hjelmn
Copy link
Contributor Author

hjelmn commented Nov 30, 2020

Dropping llvm from the dependency list helps. Will see how far that gets since ghc 8.8.4 would always fail it it couldn't locate llc (it also complained about opt missing). 8.10 may be more forgiving if it isn't a full llvm install. If this works I will get the ghc.rb change up in a PR today.

@hjelmn
Copy link
Contributor Author

hjelmn commented Nov 30, 2020

Looks like it will work with the system compiler. Forgot the make clean step so failed on the full build. Added the missing step and attempting again. Getting close to a working ghc formula.

@hjelmn
Copy link
Contributor Author

hjelmn commented Nov 30, 2020

🍺 /opt/homebrew/Cellar/ghc/8.10.2_2: 6,889 files, 1.9GB, built in 157 minutes 40 seconds

$ file /opt/homebrew/Cellar/ghc/8.10.2_2/lib/ghc-8.10.2/bin/ghc
/opt/homebrew/Cellar/ghc/8.10.2_2/lib/ghc-8.10.2/bin/ghc: Mach-O 64-bit executable arm64

@fxcoudert
Copy link
Member

Great!
Can this be closed?

@hjelmn
Copy link
Contributor Author

hjelmn commented Nov 30, 2020

Yeah. ghc 8.8 is not worth fixing at this time.

@hjelmn hjelmn closed this Nov 30, 2020
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 31, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
legacy Relates to a versioned @ formula outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants