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

macOS Apple Silicon support #415

Merged
merged 9 commits into from
May 19, 2021
Merged

Conversation

atteneder
Copy link
Contributor

@atteneder atteneder commented May 3, 2021

This patch enables building macOS binaries that support Apple Silicon (arm64) platform or universal binaries supporting both Intel (x86_64) and Apple Silicon Macs.

There's a catch:
Basis Universal SSE support has to be disabled, in arm builds and even in universal builds. SSE support is detected at configuration time, but a fat arm/intel binary would require detecting it at compile time per architecture.

I've changed the CI build to create universal binaries and disabled SSE for now Alternative solutions could be:

  • Dedicated binaries per architecture
  • Compile time SSE detection in BasisU

I've mentioned this to BasisU in this comment.

@MarkCallow
Copy link
Collaborator

MarkCallow commented May 4, 2021

You need to fix the Windows build. The transcodetest build is exiting due to a multiply defined symbol in basisu_c_binding.lib and ktx.lib. It's the etc1s global codebook.

There is a cmake/c program to detect the cpu type: cmake/cputypetest.cmake. It needs fixing to recognize the M1. Clearly it is currently returning x86_64 because the SSE option in CMake is disabled for every other case. Please look at fixing it. It's a bit of a hack right now for reasons explained in comments there.

@MarkCallow
Copy link
Collaborator

When I switched the CI build to Xcode 12.4 it started overrunning Travis's 4MB log file limit so I changed it to use xcpretty to reduce the amount of output. Just as well as we'll have even more builds after this. If with these additional builds it overruns even with xcpretty we may have to resort to xcodebuild -quiet. I think this can be passed from cmake like this

cmake --build . --config Release -- -quiet

@MarkCallow
Copy link
Collaborator

As well as fixing the Windows build, please add info about building for the M1 to BUILDING.md. Do we need to change anything in the iOS build for the new m1 iPad?

One thing that comes to mind for both macOS and iOS is the set of transcode targets supported. The M1 supports all modern formats and PVRTC so we should include all transcoders. It will be good for testing.

@lexaknyazev
Copy link
Member

One thing that comes to mind for both macOS and iOS is the set of transcode targets supported. The M1 supports all modern formats and PVRTC so we should include all transcoders. It will be good for testing.

For whatever reason, iOS on M1 does not support BC* formats.

atteneder added 5 commits May 17, 2021 15:31
…ject libraries, so I turned them to static libraries.
…hitecture while the CLI tools and libraries are still multi-arch
…ilds, because Apple Silicon (arm64) won't compile. The disadvantage is that the Intel (x86_64) architecture won't support it either.

see BinomialLLC/basis_universal#123 (comment)
@atteneder
Copy link
Contributor Author

You need to fix the Windows build. The transcodetest build is exiting due to a multiply defined symbol in basisu_c_binding.lib and ktx.lib. It's the etc1s global codebook.

I think I found a way around this difference in linking behaviour.. let's see what the CI results in.

There is a cmake/c program to detect the cpu type: cmake/cputypetest.cmake. It needs fixing to recognize the M1. Clearly it is currently returning x86_64 because the SSE option in CMake is disabled for every other case. Please look at fixing it. It's a bit of a hack right now for reasons explained in comments there.

I agree, but I don't see how this can be fixed at configuration time (only) for universal libraries. That's why I think BasisU should do some sort of compile time feature detection.

@atteneder
Copy link
Contributor Author

As well as fixing the Windows build, please add info about building for the M1 to BUILDING.md

I'll look at it.

Do we need to change anything in the iOS build for the new m1 iPad?

My rather uninformed gut feeling answer would be no. and I feel a bit confirmed by @lexaknyazev saying iOS/M1 doesn't support BC*.

One thing that comes to mind for both macOS and iOS is the set of transcode targets supported. The M1 supports all modern formats and PVRTC so we should include all transcoders. It will be good for testing.

Good point. I'll have a look.

@atteneder
Copy link
Contributor Author

As well as fixing the Windows build, please add info about building for the M1 to BUILDING.md

I'll look at it.

Done

One thing that comes to mind for both macOS and iOS is the set of transcode targets supported. The M1 supports all modern formats and PVRTC so we should include all transcoders. It will be good for testing.

Good point. I'll have a look.

As far as I can see there is no configuration time preselection of supported transcode targets (except for WebGL).

It would be good if we can make a smart per-platform preselection and offer CMake options that users can tweak, but:

  • For macOS universal this would be a shared set for both ARM and Intel though, I'm afraid.
  • I consider this another feature and out of scope of this PR

@atteneder
Copy link
Contributor Author

There is a cmake/c program to detect the cpu type: cmake/cputypetest.cmake. It needs fixing to recognize the M1. Clearly it is currently returning x86_64 because the SSE option in CMake is disabled for every other case. Please look at fixing it. It's a bit of a hack right now for reasons explained in comments there.

I agree, but I don't see how this can be fixed at configuration time (only) for universal libraries. That's why I think BasisU should do some sort of compile time feature detection.

I added detection of Apple Silicon to this script. This is now used to prevent cross compiling load tests apps on M1 to Intel. Still, this only detects the host's architecture and not the target architecture(s).

Copy link
Collaborator

@MarkCallow MarkCallow left a comment

Choose a reason for hiding this comment

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

Overall looks good except from several typos in BUILDING.md. There are also a couple of questions in the review. The one on build_macos.sh definitely needs an answer.

BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
ci_scripts/build_macos.sh Show resolved Hide resolved
@MarkCallow MarkCallow merged commit ebab2ea into KhronosGroup:master May 19, 2021
@atteneder atteneder deleted the apple_silicon branch June 10, 2021 17:21
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 21, 2024
Caveats:

* Load tests apps only support Intel macs.
* SSE support is disabled in Universal binary builds due to absence of run-time detection.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
Caveats:

* Load tests apps only support Intel macs.
* SSE support is disabled in Universal binary builds due to absence of run-time detection.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
Caveats:

* Load tests apps only support Intel macs.
* SSE support is disabled in Universal binary builds due to absence of run-time detection.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
Caveats:

* Load tests apps only support Intel macs.
* SSE support is disabled in Universal binary builds due to absence of run-time detection.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
Caveats:

* Load tests apps only support Intel macs.
* SSE support is disabled in Universal binary builds due to absence of run-time detection.
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.

3 participants