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

Sindhu/bfloat16 support #399

Merged
merged 29 commits into from
Jan 30, 2020
Merged

Sindhu/bfloat16 support #399

merged 29 commits into from
Jan 30, 2020

Conversation

sindhu-nervana
Copy link
Contributor

@sindhu-nervana sindhu-nervana commented Dec 20, 2019

Add support in bridge for TF Graph with Ops that take in bfloat data type inputs.

  • The bfloatOps are by default assigned XLA CPU device by TF. We register dummy kernels for CPU device, for bfloat data types. which makes TF assign device CPU to bfloatOps.

sindhu-nervana and others added 4 commits December 11, 2019 14:30
- Enabled --var build to use parallel executor integrating weights-on-device and data pipelining
- moved ngraph_var files outside the var build
@dbonner
Copy link

dbonner commented Jan 19, 2020

Will your work help with this issue #447 ?
The PLAIDML backend won't build because BLFOAT16 enum case is not dealt with in switch (dt).

REGISTER_NGRAPH_STUB_KERNEL("NGraphApplyMomentum");
REGISTER_NGRAPH_STUB_KERNEL(
"NGraphAssignAdd"); //*input[0] = *input[0] + input[1]
REGISTER_NGRAPH_STUB_KERNEL(
Copy link
Contributor

Choose a reason for hiding this comment

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

REGISTER_NGRAPH_STUB_KERNEL registers only for bfloat type? since its defn is:

#define REGISTER_NGRAPH_STUB_KERNEL(optype)                          \
  REGISTER_KERNEL_BUILDER(                                           \
      Name(optype).Device(DEVICE_CPU).TypeConstraint<bfloat16>("T"), \
      NGStubOp);

in that case is this replacement ok?

Copy link
Contributor

@shresthamalik shresthamalik Jan 29, 2020

Choose a reason for hiding this comment

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

changed the macros. the names were not matching the definition.

@@ -223,6 +223,9 @@ Status TensorToStream(std::ostream& ostream, const Tensor& tensor) {
case DT_BOOL:
TensorDataToStream<bool>(ostream, n_elements, data);
break;
case DT_BFLOAT16:
TensorDataToStream<bool>(ostream, n_elements, data);
Copy link
Contributor

@sayantan-nervana sayantan-nervana Jan 29, 2020

Choose a reason for hiding this comment

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

It says <bool> in the template. copy-paste error perhaps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. Not sure what the corresponding data type for bfloat is.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can throw an error or return a bad status for now I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@kanvi-nervana kanvi-nervana left a comment

Choose a reason for hiding this comment

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

Minor comments

Comment on lines 59 to 64
// Since nGraph-bridge OPs work on TF DEVICE_CPU we are registering stub
// bfloat16
// kernels here. The expectation is when we register the stub kernels for
// bfloat16
// TF is going to assign DEVICE_CPU to the respective Ops and we will
// encapsulate them
Copy link
Contributor

Choose a reason for hiding this comment

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

// Since nGraph-bridge OPs work on TF DEVICE_CPU we are registering stub
// bfloat16 kernels here. The expectation is when we register the stub kernels
// for bfloat16 TF is going to assign DEVICE_CPU to the respective Ops and
// we will encapsulate them

@@ -0,0 +1,131 @@
# ==============================================================================
# Copyright 2019 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright 2019-2020 Intel Corporation

@sayantan-nervana sayantan-nervana added the ready to merge This PR is the next in the queue. label Jan 30, 2020
@shresthamalik shresthamalik removed their request for review January 30, 2020 20:08
Copy link
Contributor

@shresthamalik shresthamalik left a comment

Choose a reason for hiding this comment

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

LGTM

@shresthamalik shresthamalik merged commit 310ca25 into master Jan 30, 2020
@shresthamalik shresthamalik deleted the sindhu/bfloat16_support branch January 30, 2020 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR is the next in the queue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants