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

Improve support for Apple Silicon and macOS #275

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thorpe-dev
Copy link
Contributor

Will file internal issue - this was done on my personal M1 laptop

  • Use clock_gettime_nsec_np over mach_absolute_time
  • Extend support for ARM CPUs to include ARM v8 (which includes Apple Silicon) and ARM v9 (to a lesser extent)
  • Fix a few places where "CPU != X86" has been considered to be a big-endian processor
  • use fstat/stat in place of fstat64/stat64 on macOS, as the latter were deprecated in macOS 10.6, and are not available on Apple Silicon
  • Fix alignment of doubles on arm
  • Disable JIT support for bdlpcre_regex: there is upstream work to be done (ref sys: disable jit on apple aarch64 BurntSushi/rust-pcre2#16 and others)

Testing performed

  • Existing unit tests, some of which still fail
  • bsls_unspecifiedbool.t.cpp fails to build at all
Failing unit tests
The following tests FAILED:
	106 - balm_publicationscheduler.t (Failed)
	117 - balst_stacktraceprintutil.t (Failed)
	160 - balxml_minireader.t (Failed)
	324 - bdldfp_decimalimputil.t (Failed)
	327 - bdldfp_decimalutil.t (Failed)
	359 - bdlma_concurrentmultipool.t (Failed)
	373 - bdlma_multipool.t (Failed)
	395 - bdls_memoryutil.t (Failed)
	399 - bdls_processutil.t (Failed)
	532 - bslim_formatguard.t (Failed)
	635 - bslmf_istransparentpredicate.t (Failed)
	706 - bslmt_threadutil.t (Failed)
	711 - bslmt_timedsemaphore.t (Failed)
	787 - bsls_stopwatch.t (Failed)
	793 - bsls_unspecifiedbool.t (Failed)
	811 - bslstl_chrono.t (Failed)
	822 - bslstl_function.t (Failed)
	824 - bslstl_function_invokerutil.t (Failed)
	881 - bslstl_stringview.t (Failed)
bsls_unspecifiedbool.t.cpp build
/Users/michaelthorpe/programming/bde/groups/bsl/bsls/bsls_unspecifiedbool.t.cpp:418:34: error: type 'BoolType' (aka 'int BloombergLP::bsls::UnspecifiedBool<int>::*') cannot be narrowed to 'bool' in initializer list [-Wc++11-narrowing]
            const bool bar[] = { bt };
                                 ^~
/Users/michaelthorpe/programming/bde/groups/bsl/bsls/bsls_unspecifiedbool.t.cpp:418:34: note: insert an explicit cast to silence this issue
            const bool bar[] = { bt };
                                 ^~
                                 static_cast<bool>( )
/Users/michaelthorpe/programming/bde/groups/bsl/bsls/bsls_unspecifiedbool.t.cpp:424:23: error: type 'BoolType' (aka 'int BloombergLP::bsls::UnspecifiedBool<int>::*') cannot be narrowed to 'bool' in initializer list [-Wc++11-narrowing]
            } bag = { bt };
                      ^~
/Users/michaelthorpe/programming/bde/groups/bsl/bsls/bsls_unspecifiedbool.t.cpp:424:23: note: insert an explicit cast to silence this issue
            } bag = { bt };
                      ^~
                      static_cast<bool>( )
/Users/michaelthorpe/programming/bde/groups/bsl/bsls/bsls_unspecifiedbool.t.cpp:621:34: error: type '(anonymous namespace)::Booleable::BoolType' (aka 'int BloombergLP::bsls::UnspecifiedBool<(anonymous namespace)::Booleable>::*') cannot be narrowed to 'bool' in initializer list [-Wc++11-narrowing]
            const bool bar[] = { babel };
                                 ^~~~~
/Users/michaelthorpe/programming/bde/groups/bsl/bsls/bsls_unspecifiedbool.t.cpp:621:34: note: insert an explicit cast to silence this issue
            const bool bar[] = { babel };
                                 ^~~~~
                                 static_cast<bool>( )
/Users/michaelthorpe/programming/bde/groups/bsl/bsls/bsls_unspecifiedbool.t.cpp:627:23: error: type '(anonymous namespace)::Booleable::BoolType' (aka 'int BloombergLP::bsls::UnspecifiedBool<(anonymous namespace)::Booleable>::*') cannot be narrowed to 'bool' in initializer list [-Wc++11-narrowing]
            } bag = { babel };
                      ^~~~~
/Users/michaelthorpe/programming/bde/groups/bsl/bsls/bsls_unspecifiedbool.t.cpp:627:23: note: insert an explicit cast to silence this issue
            } bag = { babel };
                      ^~~~~
                      static_cast<bool>( )
4 errors generated.

@mversche
Copy link
Contributor

This overlaps with #273

Copy link

@notadragon notadragon left a comment

Choose a reason for hiding this comment

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

Overall this seems good, well past our baseline target of just compiling on M1. If you can address my feedback (and verify that things still work on your M1) then we can check this PR on other platforms and land it.

@@ -911,6 +919,8 @@ struct Platform {
struct CpuArmv5 : CpuArm {};
struct CpuArmv6 : CpuArm {};
struct CpuArmv7 : CpuArm {};
struct CpuArmv8 : CpuArm {};
struct CpuArmv9 : CpuArm {};

Choose a reason for hiding this comment

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

v9 seems unused, we can remove that.

@@ -448,114 +448,57 @@ struct MachTimerUtil {

private:

Choose a reason for hiding this comment

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

This is fairly extensive. Looking at it I think it's fine, but needs a few changes (which i can't comment on directly because of github):

  1. I suspect the include of <mach/mach_time.h> is no longer the right include... it looks like it should just be <time.h>?

  2. Please change "MachTimerUtil" to "DarwinTimerUtil".

It seems based on documentation that this should also work on non-m1 darwin machines, but we'll have to try to verify that.

@@ -121,7 +121,7 @@ static u64 u8to64_le(const u8* p)
{
BSLS_ASSERT(p);

#if defined(BSLS_PLATFORM_CPU_X86) || defined(BSLS_PLATFORM_CPU_X86_64)
#if defined(BSLS_PLATFORM_IS_LITTLE_ENDIAN)
return *reinterpret_cast<const u64 *>(p); // Ignore alignment.

Choose a reason for hiding this comment

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

What about alignment?

Choose a reason for hiding this comment

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

Huh, seems AArch64 typically allows unaligned accesses in user space.

Choose a reason for hiding this comment

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

Actually, that is a good point - i wouldn't widen the existing assumption to all little endian machines.

@@ -380,7 +380,7 @@ SpookyHashAlgorithm::SpookyHashAlgorithm()
inline
SpookyHashAlgorithm::SpookyHashAlgorithm(const char *seed)
: d_state(
#if !defined(BSLS_PLATFORM_CPU_X86_64) && !defined(BSLS_PLATFORM_CPU_X86)
#if !defined(BSLS_PLATFORM_IS_LITTLE_ENDIAN)

Choose a reason for hiding this comment

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

This should also not be checking for little_endian - this is about whether the cpu handles unaligned reads properly. For now i would leave this as-is and we can engage the optimization at a later date if we make the m1 a more fully supported platform.

@emeryberger
Copy link

emeryberger commented Feb 5, 2022

Built from this branch on my Mac Mini M1. All I needed to do to get bsls_unspecifiedbool.t.cpp was adding static_cast<bool> around bt and babel; good error messages for a change!

I had an almost completely different set of failed unit tests. - denotes the tests that failed given in @thorpe-dev's top comment; + denotes the tests that failed in my run.

Click to see the diff
The following tests FAILED:
             106 - balm_publicationscheduler.t (Failed)
     -       117 - balst_stacktraceprintutil.t (Failed)
     -       160 - balxml_minireader.t (Failed)
     -       324 - bdldfp_decimalimputil.t (Failed)
     -       327 - bdldfp_decimalutil.t (Failed)
     -       359 - bdlma_concurrentmultipool.t (Failed)
     -       373 - bdlma_multipool.t (Failed)
     -       395 - bdls_memoryutil.t (Failed)
     -       399 - bdls_processutil.t (Failed)
     -       532 - bslim_formatguard.t (Failed)
     -       635 - bslmf_istransparentpredicate.t (Failed)
     -       706 - bslmt_threadutil.t (Failed)
     -       711 - bslmt_timedsemaphore.t (Failed)
     -       787 - bsls_stopwatch.t (Failed)
     -       793 - bsls_unspecifiedbool.t (Failed)
     -       811 - bslstl_chrono.t (Failed)
     -       822 - bslstl_function.t (Failed)
     -       824 - bslstl_function_invokerutil.t (Failed)
     -       881 - bslstl_stringview.t (Failed)
     +       118 - balst_stacktraceprintutil.t (Failed)
     +       161 - balxml_minireader.t (Failed)
     +       274 - bdlcc_boundedqueue.t (Failed)
     +       325 - bdldfp_decimalimputil.t (Failed)
     +       328 - bdldfp_decimalutil.t (Failed)
     +       360 - bdlma_concurrentmultipool.t (Failed)
     +       374 - bdlma_multipool.t (Failed)
     +       396 - bdls_memoryutil.t (Failed)
     +       400 - bdls_processutil.t (Failed)
     +       534 - bslim_formatguard.t (Failed)
     +       637 - bslmf_istransparentpredicate.t (Failed)
     +       713 - bslmt_timedsemaphore.t (Timeout)
     +       789 - bsls_stopwatch.t (Failed)
     +       813 - bslstl_chrono.t (Failed)
     +       824 - bslstl_function.t (Failed)
     +       826 - bslstl_function_invokerutil.t (Failed)
     +       883 - bslstl_stringview.t (Failed)

Platform: macOS Monterey Version 12.1, Mac mini (M1, 2020)

Darwin minim 21.2.0 Darwin Kernel Version 21.2.0: Sun Nov 28 20:29:10 PST 2021; root:xnu-8019.61.5~1/RELEASE_ARM64_T8101 arm64

Apple clang version 13.0.0 (clang-1300.0.29.30)
Target: arm64-apple-darwin21.2.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

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.

5 participants