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

Switch to gcc atomic intrinsics for macOS and delete the file that uses the deprecated atomics #2699

Merged
merged 1 commit into from Feb 10, 2017

Conversation

Projects
None yet
4 participants
@jhseu
Contributor

jhseu commented Feb 9, 2017

The static_assert on the sizeof std::atomic fails with nvcc. Use the gcc atomic intrinsics instead, which are supported in both gcc and clang.

Manually tested that this fixes the nvcc build.

@sergiocampama

@grpc-jenkins

This comment has been minimized.

Show comment
Hide comment
@grpc-jenkins

grpc-jenkins Feb 9, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-jenkins commented Feb 9, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-jenkins

This comment has been minimized.

Show comment
Hide comment
@grpc-jenkins

grpc-jenkins Feb 9, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-jenkins commented Feb 9, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@googlebot googlebot added the cla: yes label Feb 9, 2017

@jhseu

This comment has been minimized.

Show comment
Hide comment
@jhseu

jhseu Feb 9, 2017

Contributor

Primarily needed to fix the GPU build for TensorFlow on macOS.

Contributor

jhseu commented Feb 9, 2017

Primarily needed to fix the GPU build for TensorFlow on macOS.

@jhseu jhseu changed the title from Fix the build on macOS with nvcc to Switch to gcc atomic intrinsics for macOS Feb 10, 2017

@jhseu jhseu changed the title from Switch to gcc atomic intrinsics for macOS to Switch to gcc atomic intrinsics for macOS and delete the file that uses the deprecated atomics Feb 10, 2017

@@ -194,14 +194,6 @@ Atomic64 Release_Load(volatile const Atomic64* ptr);
#elif defined(GOOGLE_PROTOBUF_OS_AIX)
#include <google/protobuf/stubs/atomicops_internals_power.h>
// Apple.

This comment has been minimized.

@TeBoring

TeBoring Feb 10, 2017

Contributor

Does it still pass on older version of mac

@TeBoring

TeBoring Feb 10, 2017

Contributor

Does it still pass on older version of mac

This comment has been minimized.

@jhseu

jhseu Feb 10, 2017

Contributor

I don't have access to an older mac, but the intrinsics work on clang/llvm 3.4 from 2014 (tested on my desktop). Looking at commits, should be fine for versions from 2011 forward. How old do we want to support?

@jhseu

jhseu Feb 10, 2017

Contributor

I don't have access to an older mac, but the intrinsics work on clang/llvm 3.4 from 2014 (tested on my desktop). Looking at commits, should be fine for versions from 2011 forward. How old do we want to support?

This comment has been minimized.

This comment has been minimized.

@TeBoring

TeBoring Feb 10, 2017

Contributor

You may need to comment out the c++11 flag in configure.ac

@TeBoring

TeBoring Feb 10, 2017

Contributor

You may need to comment out the c++11 flag in configure.ac

This comment has been minimized.

@jhseu

jhseu Feb 10, 2017

Contributor

Tested with make clean && ./configure CXXFLAGS="-mmacosx-version-min=10.7" && make and it doesn't affect the versions the existing code builds at:
>= 10.9 required with C++11 set
>= 10.7 required when deleting it from configure.ac

@jhseu

jhseu Feb 10, 2017

Contributor

Tested with make clean && ./configure CXXFLAGS="-mmacosx-version-min=10.7" && make and it doesn't affect the versions the existing code builds at:
>= 10.9 required with C++11 set
>= 10.7 required when deleting it from configure.ac

@TeBoring

This comment has been minimized.

Show comment
Hide comment
@TeBoring

TeBoring Feb 10, 2017

Contributor

LGTM. Could you squash commits into a single one?

Contributor

TeBoring commented Feb 10, 2017

LGTM. Could you squash commits into a single one?

@jhseu

This comment has been minimized.

Show comment
Hide comment
@jhseu

jhseu Feb 10, 2017

Contributor

Done.

FYI, in TensorFlow, we default to squash when merging pull requests:
https://github.com/blog/2141-squash-your-commits

Contributor

jhseu commented Feb 10, 2017

Done.

FYI, in TensorFlow, we default to squash when merging pull requests:
https://github.com/blog/2141-squash-your-commits

@TeBoring TeBoring merged commit ef927cc into protocolbuffers:master Feb 10, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci.bazel.io Build finished.
Details
cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

dopuskh3 added a commit to criteo-forks/protobuf that referenced this pull request Feb 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment