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
Mkl kernels #8184
Mkl kernels #8184
Conversation
Can one of the admins verify this patch? |
@petewarden / @josh11b could one of you take a look? |
|
||
if (input_is_mkl) { | ||
OP_REQUIRES(context, mkl_params_.mkl_input_shape.GetDimension() == 4, | ||
errors::InvalidArgument("Input tensor must be 4-dimensional")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing looks funny.
limitations under the License. | ||
==============================================================================*/ | ||
|
||
// See docs in ../ops/nn_ops.cc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize there is a lot of documentation in nn_ops.cc to which this comment refers. However, could you add comments here that explains how this OpKernel differs from those in 'nn_ops.cc' (it uses mkl library, it changes layout, etc). It would be good to have a high-level description at the top on this file (as well as backprop input).
MklCreateInputLayouts(context); | ||
|
||
Tensor mkl_tmp_input_buf, mkl_tmp_outbackprop_buf_; | ||
MklPrepareConvolutionInputs(context, &mkl_tmp_input_buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad you broke out function calls for MklPrepare* and MklCleanup...
#include "third_party/mkl/include/mkl_dnn_types.h" | ||
|
||
namespace tensorflow { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above: high-level description up top and some comments about what each method does.
int64 pad_top, pad_bottom; | ||
int64 pad_left, pad_right; | ||
OP_REQUIRES_OK(context, GetWindowedOutputSizeVerbose( | ||
dims.rows.input_size, dims.rows.filter_size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line spacing looks weird.
.SetShapeFn([](InferenceContext* c) { | ||
return InputTensorShapeOrUnknown(c, 0 /* input_idx */, 4 /* ndims */); | ||
}) | ||
.Doc(R"doc( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document all inputs, outputs and attrs. Please look at other doc strings as examples.
|
||
mkl_params.in_dims = 4; | ||
|
||
mkl_params.in_sizes[0] = static_cast<size_t>(dims.cols.input_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe break this up with line spaces between param types for readability:
mkl_params.input_offset[0] = static_cast<int>(-pad_left);
mkl_params.input_offset[1] = static_cast<int>(-pad_top);
 mkl_params.conv_strides[0] = static_cast<size_t>(dims.cols.stride);
 mkl_params.conv_strides[1] = static_cast<size_t>(dims.rows.stride);
tensorflow/core/util/mkl_util.h
Outdated
sizes.push_back(tf_sizes[2]); | ||
} else { | ||
sizes.push_back(tf_sizes[2]); | ||
sizes.push_back(tf_sizes[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix line spacing.
Looks good, but please address my comments above before committing. |
@tensorflow-jenkins test this please |
Can one of the admins verify this patch? |
@tensorflow-jenkins test this please |
Can you address the build failure:
|
Code review comments should be addressed now, and the immutable op test failure should be fixed. |
@tensorflow-jenkins test this please |
The failure here seems unrelated to our work (likely a setup issue):
|
@Cais do you recognize this failure?
|
@martinwicke I believe you pinged the wrong person ... |
Ah, sorry, I did! @caisq, do you recognize this failure? |
@martinwicke Yes, this is probably because pyreadline (the Windows equivalent of readline), which is a dependency of tfdbg on Windows, is not installed on the particular Windows machine (win1-slave). I'll install it. This error shouldn't block this PR. |
Thank you! |
Adding MKL support for conv_ops