Adding support for Apple M1 AArch64 Architecture Processor. #150
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
| @@ -146,7 +156,7 @@ set_property(TARGET cpu_features PROPERTY POSITION_INDEPENDENT_CODE ${BUILD_PIC} | |||
| target_include_directories(cpu_features | |||
| PUBLIC $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/cpu_features> | |||
| ) | |||
| if(PROCESSOR_IS_X86) | |||
| if(PROCESSOR_IS_X86 OR PROCESSOR_IS_AARCH64) | |||
There was a problem hiding this comment.
I know you're following the original code here, but it would make more sense to instead do just a single if here since the clauses are just about APPLE, e.g.:
if(APPLE AND (PROCESSOR_IS_X86 OR PROCESSOR_IS_AARCH64))
There was a problem hiding this comment.
Good point. I'm not great with Cmake yet. I'm sure there will be more changes, but currently it passes the build checks so I'll probably make the change locally and wait on it to submit until we get any more feedback.
|
Unfortunately it looks like Apple doesn't expose everything — running In Parallels virtualization of Ubuntu 20.04 it shows It might be necessary to store tables of capabilities that match on the LLVM has such a table here (currently covering A7 through A13): |
This is my digested version of the table from llvm. HasV8_2aOps means supports all of AARCH 8.2 so I have filled in the rows based on those too. I'm not sure how to map the names to the names here though. M1 is based on A14, which may have further differences to A13 that are not in llvm upstream yet. |
| set(PROCESSOR_IS_X86 TRUE) | ||
| elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^(powerpc|ppc)") | ||
| set(PROCESSOR_IS_POWER TRUE) | ||
| endif() | ||
| endif() | ||
|
|
||
| macro(add_cpu_features_headers_and_sources HDRS_LIST_NAME SRCS_LIST_NAME) |
There was a problem hiding this comment.
I've been editing things to get this building on M1 (actually Universal) and I had to change the way files are included because generating a project as it stands currently, it'll pick one of the architecture's sets of files to include, but then if you build as universal it obviously does not have the required files for the other architecture. So I changed it locally so that all the files are always included and then added #if per platform in the various platform files.
There was a problem hiding this comment.
Oh, and there are also issues with including the libraries because it decides based on the host architecture for some of it (unix_based_hardware_detection) so it can fail to build in those cases too.
There was a problem hiding this comment.
Yeah CMake universal builds are a bit of a pain. They are 1 build, making universal targets, so anything done at configure time is probably incorrect. I found that you basically have to move all this stuff ot compile time with ifdefs.
There was a problem hiding this comment.
my 2 cents,
CMAKE_SYSTEM_PROCESSOR is for target system and it's usually part of a cmake cross-toolchain otherwise you have the CMAKE_HOST_SYSTEM_PROCESSOR...
ref: https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_PROCESSOR.html
For universal platform, i don't know if we could/should introduce a "universal" cmake system processor and propagate it accordingly than including everything and using preprocessor everywhere...
There was a problem hiding this comment.
The way to build for universal currently means invoking xcodebuild with dual architecture. This is well after cmake has decided which files to include. I know the CMake guys have been doing some work on all this, but its still early days. Think we just have to suck it up for now.
Besides, I've always found writing cross-platform code is far easier when its all visible in a project.
There was a problem hiding this comment.
The way to build for universal currently means invoking xcodebuild with dual architecture. This is well after cmake has decided which files to include.
I means, instead of adding all files we could simply add files supported by their "universal 2" build, I'm not sure Apple support (mips, ppc, arm, arm64 and x86)...
EDIT: seems there is universal(i386,ppc) and universal2 (x86_64; arm64)
ref: https://developer.apple.com/documentation/xcode/building_a_universal_macos_binary
I know the CMake guys have been doing some work on all this, but its still early days. Think we just have to suck it up for now.
I think you must talk about CMAKE_OSX_ARCHITECTURES
There was a problem hiding this comment.
Yeah, I've tried using that to target a specific target and that's ok, but it does not quite work yet. ie. you can't tell it x86_64;arm64 as it complains. Hence the xcodebuild route.
|
There is arm A14 added to llvm if it helps in any way. https://github.com/apple/llvm-project/blob/apple/main/llvm/lib/Target/AArch64/AArch64.td#L757 |
|
@IanMDay is the universal2 route still the way to go here? If that's not working, could this patchset be used as a basis for a non-universal2 mac build? |
I've not revisited it since then, sorry. |
There was a problem hiding this comment.
There are changes that would make this PR more concise, but I do think it works as-is. I've added a patch to the uhd-devel port in MacPorts that is based on this PR, to get Volk working on Apple ARM64 / M1 in at least a basic sense. This PR is a good starting point if nothing else.
I am not addressing the Universal2 issue being discussed. That's a separate issue to me. I just want support for the single new ARM64 architecture.
| set(HOST_ARCHITECTURE "${arch}") | ||
| string(TOLOWER ${HOST_ARCHITECTURE} HOST_ARCHITECTURE) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
This is not the correct location for this code, and at least with CMake 3.21 this code also isn't necessary for Apple ARM64 support; CMAKE_SYSTEM_PROCESSOR is correctly set to arm64 just as you're doing here. IDK about prior CMake, except that the variable CMAKE_SYSTEM_PROCESSOR has been around for a long time and seems to represent what you're trying to do here.
There was a problem hiding this comment.
For the record, CMake as added the M1 support in 3.19 according to https://cmake.org/cmake/help/latest/release/3.19.html#platforms
There was a problem hiding this comment.
While that may be true, still with CMake 3.22.0, I get this build error on an M1 MacBook Pro:
npm ERR! gyp info using node-gyp@8.4.1
npm ERR! gyp info using node@17.2.0 | darwin | arm64
npm ERR! gyp info find Python using Python version 3.9.9 found at "/opt/homebrew/opt/python@3.9/bin/python3.9"
npm ERR! gyp info spawn /opt/homebrew/opt/python@3.9/bin/python3.9
npm ERR! gyp info spawn args [
npm ERR! gyp info spawn args '/Users/jwatte/source/repos/cloud-instance/node_modules/node-gyp/gyp/gyp_main.py',
npm ERR! gyp info spawn args 'binding.gyp',
npm ERR! gyp info spawn args '-f',
npm ERR! gyp info spawn args 'make',
npm ERR! gyp info spawn args '-I',
npm ERR! gyp info spawn args '/Users/jwatte/source/repos/cloud-instance/node_modules/cpu-features/build/config.gypi',
npm ERR! gyp info spawn args '-I',
npm ERR! gyp info spawn args '/Users/jwatte/source/repos/cloud-instance/node_modules/node-gyp/addon.gypi',
npm ERR! gyp info spawn args '-I',
npm ERR! gyp info spawn args '/Users/jwatte/Library/Caches/node-gyp/17.2.0/include/node/common.gypi',
npm ERR! gyp info spawn args '-Dlibrary=shared_library',
npm ERR! gyp info spawn args '-Dvisibility=default',
npm ERR! gyp info spawn args '-Dnode_root_dir=/Users/jwatte/Library/Caches/node-gyp/17.2.0',
npm ERR! gyp info spawn args '-Dnode_gyp_dir=/Users/jwatte/source/repos/cloud-instance/node_modules/node-gyp',
npm ERR! gyp info spawn args '-Dnode_lib_file=/Users/jwatte/Library/Caches/node-gyp/17.2.0/<(target_arch)/node.lib',
npm ERR! gyp info spawn args '-Dmodule_root_dir=/Users/jwatte/source/repos/cloud-instance/node_modules/cpu-features',
npm ERR! gyp info spawn args '-Dnode_engine=v8',
npm ERR! gyp info spawn args '--depth=.',
npm ERR! gyp info spawn args '--no-parallel',
npm ERR! gyp info spawn args '--generator-output',
npm ERR! gyp info spawn args 'build',
npm ERR! gyp info spawn args '-Goutput_dir=.'
npm ERR! gyp info spawn args ]
npm ERR! gyp info spawn make
npm ERR! gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build' ]
npm ERR! In file included from /Users/jwatte/source/repos/cloud-instance/node_modules/cpu-features/deps/cpu_features/src/cpuinfo_arm.c:15:
npm ERR! /Users/jwatte/source/repos/cloud-instance/node_modules/cpu-features/deps/cpu_features/include/cpuinfo_arm.h:118:2: error: "Including cpuinfo_arm.h from a non-arm target."
npm ERR! #error "Including cpuinfo_arm.h from a non-arm target."
npm ERR! ^
npm ERR! 1 error generated.
npm ERR! make[3]: *** [CMakeFiles/cpu_features.dir/src/cpuinfo_arm.c.o] Error 1
| elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^arm") | ||
| set(PROCESSOR_IS_ARM TRUE) | ||
| elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^aarch64") | ||
| if(HOST_ARCHITECTURE MATCHES "^arm64") |
There was a problem hiding this comment.
Removing the above code, this chunk can be reverted back to original ... with 2 changes as noted below
| @@ -146,7 +146,7 @@ flags : aes,avx,cx16,smx,sse4_1,sse4_2,ssse3 | |||
| | Android | yes² | yes¹ | yes¹ | yes¹ | N/A | | |||
| | iOS | N/A | not yet | not yet | N/A | N/A | | |||
| | Linux | yes² | yes¹ | yes¹ | yes¹ | yes¹ | | |||
| | MacOs | yes² | N/A | not yet | N/A | no | | |||
| | MacOs | yes² | N/A | yes² | N/A | no | | |||
There was a problem hiding this comment.
since we're modifying this line, let's use macOS instead of MacOs
| @@ -39,6 +39,10 @@ | |||
| #define CPU_FEATURES_ARCH_ARM | |||
| #endif | |||
|
|
|||
| #if (defined(__APPLE__) && defined(__arm64__)) | |||
There was a problem hiding this comment.
I for one would prefer to join this into the next statement, as
#if (defined(__aarch64__) || (defined(__APPLE__) && defined(__arm64__)))
| @@ -22,6 +22,35 @@ | |||
| #include "internal/stack_line_reader.h" | |||
| #include "internal/string_view.h" | |||
|
|
|||
| // The following includes are necessary to provide SSE detections on pre-AVX | |||
| // microarchitectures. | |||
| #if defined(CPU_FEATURES_OS_DARWIN) | |||
There was a problem hiding this comment.
prefer to join this #if (for CPU_FEATURES_OS_DARWIN) and the next for CPU_FEATURES_OS_DARWIN together into 1 clump. Yes, there are places elsewhere in the code where these are separate, but there is also other code between then. Here, it's simple to join them together.
| else() | ||
| if(CMAKE_SYSTEM_PROCESSOR MATCHES "^mips") | ||
| set(PROCESSOR_IS_MIPS TRUE) | ||
| elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^arm") |
| set(PROCESSOR_IS_MIPS TRUE) | ||
| elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^arm") | ||
| set(PROCESSOR_IS_ARM TRUE) | ||
| elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^aarch64") |
| @@ -146,7 +156,7 @@ set_property(TARGET cpu_features PROPERTY POSITION_INDEPENDENT_CODE ${BUILD_PIC} | |||
| target_include_directories(cpu_features | |||
| PUBLIC $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/cpu_features> | |||
| ) | |||
| if(PROCESSOR_IS_X86) | |||
| if(PROCESSOR_IS_X86 OR PROCESSOR_IS_AARCH64) | |||
Based on google/cpu_features#150 and michaelld comments
|
Can we get this merged in please? With the new 14" and 16" MBPs there is a lot more Apple Silicon floating around (including my M1 Max). I am having an issue in a child Node.js npm module that is based off this project. See mscdex/cpu-features#6 |
|
Hi @nodesocket, recently a new layout cpu features was added #194, so we need to fix merge conflicts, plus to all this, I added a new pull request that should receive cpu information regardless of the OS #186 |
Completely based on google#150. Thanks to @sbehnke! + Refactoring to accomodate the new source tree + Adding more feature flags
Completely based on google#150. Thanks to @sbehnke! + Refactoring to accomodate the new source tree + Adding more feature flags
Completely based on google#150. Thanks to @sbehnke! + Refactoring to accomodate the new source tree + Adding more feature flags
Completely based on google#150. Thanks to @sbehnke! + Refactoring to accomodate the new source tree + Adding more feature flags
Completely based on google#150. Thanks to @sbehnke! + Refactoring to accomodate the new source tree + Adding more feature flags
Completely based on google#150. Thanks to @sbehnke! + Refactoring to accomodate the new source tree + Adding more feature flags
|
@sbehnke, could you close this PR? if no objections |
To add support AARCH64 for other operating systems such as macOS(Apple M1), ios, FreeBSD, Windows has been moved common logic from `src/impl_aarch64_linux_or_android.c` to `src/impl_aarch64__base_implementation.inl`, namely: * Definitions for introspection * `Aarch64Info` kEmptyAarch64Info field Removed include "internal/bit_utils.h" from `src/impl_aarch64_linux_or_android.c`, since this include was not used. Also, include `cpuinfo_aarch64` has been removed from linux implementation and replaced with `impl_aarch64__base_implementation.inl`, this include will be used for all other operating system impl as well Added a compilation check that matches the base X86 implementation Refs: google#121 See also: google#150, google#186, google#204
To add support AARCH64 for other operating systems such as macOS(Apple M1), ios, FreeBSD, Windows has been moved common logic from `src/impl_aarch64_linux_or_android.c` to `src/impl_aarch64__base_implementation.inl`, namely: * Definitions for introspection * `Aarch64Info` kEmptyAarch64Info field Removed include "internal/bit_utils.h" from `src/impl_aarch64_linux_or_android.c`, since this include was not used. Also, include `cpuinfo_aarch64` has been removed from linux implementation and replaced with `impl_aarch64__base_implementation.inl`, this include will be used for all other operating systems impl as well Added a compilation check that matches the base X86 implementation Refs: google#121 See also: google#150, google#186, google#204
Completely based on google#150. Thanks to @sbehnke! + Refactoring to accomodate the new source tree + Adding more feature flags
Completely based on google#150. Thanks to @sbehnke! + Refactoring to accomodate the new source tree + Adding more feature flags
* Add support for Apple M1 AArch64 SoCs Completely based on #150. Thanks to @sbehnke! + Refactoring to accomodate the new source tree + Adding more feature flags * revert minimum version to 3.0 * Update introspection table * Simplify logic for Apple HAVE_SYSCTLBYNAME --------- Co-authored-by: Guillaume Chatelet <gchatelet@google.com>
This is the initial attempt at adding support for the new Apple Silicon M1 cpu to cpu_features. Based on my own laptop I tried to make my best guess for how the ARM features map from the
sysctloptional values to theAArch64Features.I also tried to map between the variant, revision, part and implementor based on the following sysctl values. I'm not sure if I got them correct, but it is a good first stab at it.
The output I get is:
This pull request is an effort to address:
#121
gnuradio/volk#428