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

Initial support for OpenCL #5267

Merged
merged 63 commits into from Nov 4, 2016

Conversation

Projects
None yet
@benoitsteiner
Member

benoitsteiner commented Oct 28, 2016

No description provided.

benoitsteiner and others added some commits Jun 3, 2016

Merge branch 'master' of https://github.com/tensorflow/tensorflow
Conflicts:
	eigen.BUILD
	tensorflow/workspace.bzl
	third_party/eigen3/Eigen/Cholesky
	third_party/eigen3/Eigen/Core
	third_party/eigen3/Eigen/Eigenvalues
	third_party/eigen3/Eigen/LU
	third_party/eigen3/Eigen/QR
	third_party/eigen3/unsupported/Eigen/CXX11/Tensor
Merge pull request #2 from lukeiwanski/ComputeCpp
ComputeCpp CE compatibility.
Merge branch 'master' of https://github.com/tensorflow/tensorflow
Conflicts:
	tensorflow/core/kernels/function_ops.cc
	tensorflow/workspace.bzl
@vrv

Cool, just a few more comments on the C++ side of things. I don't know much about the bazel stuff, so maybe @jart could take a look

Show outdated Hide outdated tensorflow/core/common_runtime/device_factory.cc
@@ -97,11 +97,20 @@ Status DeviceFactory::AddDevices(const SessionOptions& options,
gpu_factory->CreateDevices(options, name_prefix, devices));
}
// Then SYCL.

This comment has been minimized.

@vrv

vrv Nov 2, 2016

Contributor

This still isn't necessary, I think, and we should remove it.

@vrv

vrv Nov 2, 2016

Contributor

This still isn't necessary, I think, and we should remove it.

Show outdated Hide outdated tensorflow/core/common_runtime/rendezvous_mgr.cc
const bool dst_host =
(recv_args.alloc_attrs.on_host() || parsed.dst.type == "CPU");
const bool src_host = (send_args.alloc_attrs.on_host() ||
parsed.src.type == "CPU" || parsed.src.type == "SYCL");

This comment has been minimized.

@vrv

vrv Nov 2, 2016

Contributor

Can you elaborate what it works around, and where we intend to be in the future?

@vrv

vrv Nov 2, 2016

Contributor

Can you elaborate what it works around, and where we intend to be in the future?

Show outdated Hide outdated tensorflow/core/util/device_name_utils.cc
@@ -162,6 +162,16 @@ bool DeviceNameUtils::ParseFullName(StringPiece fullname, ParsedName* p) {
}
progress = true;
}
if (str_util::ConsumePrefix(&fullname, "/sycl:") ||

This comment has been minimized.

@vrv

vrv Nov 2, 2016

Contributor

We still need to remove this.

@vrv

vrv Nov 2, 2016

Contributor

We still need to remove this.

Show outdated Hide outdated third_party/eigen3/BUILD
@@ -23,5 +23,7 @@ cc_library(
"unsupported/Eigen/CXX11/FixedPoint",
],
visibility = ["//visibility:public"],
deps = ["@eigen_archive//:eigen"],
deps = ["@eigen_archive//:eigen",

This comment has been minimized.

@vrv

vrv Nov 2, 2016

Contributor

can you run buildifer on this? It's going to fail our sanity check otherwise.

@vrv

vrv Nov 2, 2016

Contributor

can you run buildifer on this? It's going to fail our sanity check otherwise.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 2, 2016

lukeiwanski pushed a commit to lukeiwanski/tensorflow-opencl that referenced this pull request Nov 2, 2016

@benoitsteiner benoitsteiner added cla: yes and removed cla: no labels Nov 3, 2016

@benoitsteiner

This comment has been minimized.

Show comment
Hide comment
@benoitsteiner

benoitsteiner Nov 3, 2016

Member

Jenkins, test this please.

Member

benoitsteiner commented Nov 3, 2016

Jenkins, test this please.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 3, 2016

Show outdated Hide outdated tensorflow/core/kernels/identity_op.cc
@@ -24,6 +24,7 @@ limitations under the License.
namespace tensorflow {
REGISTER_KERNEL_BUILDER(Name("Identity").Device(DEVICE_CPU), IdentityOp);
REGISTER_KERNEL_BUILDER(Name("Identity").Device(DEVICE_SYCL), IdentityOp);

This comment has been minimized.

@vrv

vrv Nov 3, 2016

Contributor

protect this with #if TENSORFLOW_USE_SYCL ? (same with some of the other files)

@vrv

vrv Nov 3, 2016

Contributor

protect this with #if TENSORFLOW_USE_SYCL ? (same with some of the other files)

This comment has been minimized.

@benoitsteiner

benoitsteiner Nov 3, 2016

Member

Done. I'll update the PR in the next few minutes.

@benoitsteiner

benoitsteiner Nov 3, 2016

Member

Done. I'll update the PR in the next few minutes.

@vrv

vrv approved these changes Nov 3, 2016

@vrv

This comment has been minimized.

Show comment
Hide comment
@vrv

vrv Nov 3, 2016

Contributor

Approving the C++ changes. Don't know about build changes.

Contributor

vrv commented Nov 3, 2016

Approving the C++ changes. Don't know about build changes.

@vrv vrv added cla: yes and removed cla: no labels Nov 3, 2016

@jart

This comment has been minimized.

Show comment
Hide comment
@jart

jart Nov 3, 2016

Member

I'm not sure if I'm qualified to review this. But I would like to have a better understanding of whether or not we considered alternatives to adding more stuff to the configure script. I'd also like to know how we plan to regression test this functionality.

Member

jart commented Nov 3, 2016

I'm not sure if I'm qualified to review this. But I would like to have a better understanding of whether or not we considered alternatives to adding more stuff to the configure script. I'd also like to know how we plan to regression test this functionality.

@gunan

This comment has been minimized.

Show comment
Hide comment
@gunan

gunan Nov 3, 2016

Member

At the moment, we are running "bazel test tensorflow/..." with config=sycl.
We have a few machines that can have opencl support, we will utilize those to run tests continuously.

Member

gunan commented Nov 3, 2016

At the moment, we are running "bazel test tensorflow/..." with config=sycl.
We have a few machines that can have opencl support, we will utilize those to run tests continuously.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 3, 2016

@benoitsteiner benoitsteiner added cla: yes and removed cla: no labels Nov 4, 2016

@benoitsteiner benoitsteiner merged commit f179e0a into tensorflow:master Nov 4, 2016

9 checks passed

Android Demo App SUCCESS
Details
Linux CPU Tests SUCCESS
Details
Linux CPU Tests (Python 3) SUCCESS
Details
Linux CPU Tests CMAKE SUCCESS
Details
Linux GPU SUCCESS
Details
MacOS CPU Tests SUCCESS
Details
Sanity Checks SUCCESS
Details
Windows Cmake Tests SUCCESS
Details
ci.tensorflow.org SUCCESS
Details

@bhack bhack referenced this pull request Nov 7, 2016

Open

OpenCL support #22

benoitsteiner pushed a commit to benoitsteiner/tensorflow that referenced this pull request Nov 15, 2016

benoitsteiner added a commit to benoitsteiner/tensorflow that referenced this pull request Nov 15, 2016

@rickyhan

This comment has been minimized.

Show comment
Hide comment
@rickyhan

rickyhan Nov 16, 2016

Thank you for supporting OpenCL. How can I test it out? I have a triple AMD R9 290X setup.

rickyhan commented Nov 16, 2016

Thank you for supporting OpenCL. How can I test it out? I have a triple AMD R9 290X setup.

@gunan

This comment has been minimized.

Show comment
Hide comment
@gunan

gunan Nov 16, 2016

Member

This is just the bare minimum changes we need for opencl.
Full support is still not there. Once we have full support, we will have documentation , and a few toy examples you can use.

Member

gunan commented Nov 16, 2016

This is just the bare minimum changes we need for opencl.
Full support is still not there. Once we have full support, we will have documentation , and a few toy examples you can use.

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