@yinghai @gujinghui @mingfeima @Jianhui-Li

@bddppq @gchanan @dzhulgakov @apaszke @cpuhrsch This is the PR for mkldnn tensor for C10. Your feedback are welcome.

MKLDNNTensorImpl should be put under c10 folder but since it would require c10 build including IDEEP, we will do it later as well.

SparseTensorImpl isn't in c10, why does MKLDNNTensorImpl need to be?

MKLDNNTensorImpl should be put under c10 folder but since it would require c10 build including IDEEP, we will do it later as well.

SparseTensorImpl isn't in c10, why does MKLDNNTensorImpl need to be?

@gchanan Thanks for the question. Since both ATen and Caffe2 backends need MKL-DNN optimization and are merging into c10, it would be beneficial to allow them share the same MKL-DNN tensor definition, the same way as how CPU/GPU tensors are being unified (into c10::TensorImpl). That's why c10 folder is a better place to hold MKLDNNTensorImpl.

I have a general question. When would you expect and/or plan to have mkldnn is fully available in pytorch?

@ykim362 that depends on how you define fully.

On the caffe2 backend, the 2D CNN features are full. In case you have further questions jgong5 has the answer.
On the aten backend, conv2d is already there and rnn is currently under review. Most of other operators (bn, relu, pooling, and the 3D ones) already finished coding but perhaps i still need to refactor some of them (now aten calls mkldnn directly and caffe2 calls ideep bridge).

Thanks for the answer @mingfeima !
Yes, I am referring aten backend. Would FC layer also be included?
Thanks.

@pytorchbot rebase this please

is this currently useful to expose to the eager frontend? Are there ops you can string together where converting from/to the layout inside the op isn't sufficient?

is this currently useful to expose to the eager frontend? Are there ops you can string together where converting from/to the layout inside the op isn't sufficient?

@gchanan Thanks for the question. Yes, it is important to the performance of eager frontend. It can avoid conversion overhead of activation and weight in ops related to both CNN and RNN. As an example, conversion overhead of activation accounts for 20%-50% of the training workload with ResNet-50 and UNet3D. Weight cache can speed up a particular LSTM inference workload by 1.2X to 5-6X (see #15914).

@gchanan I noticed you disabled the access to data_ptr of opaque tensor. Will you turn it on in the future? I think we can leave the flexibility for opaque tensor to leverage the storage, therefore enabling data_ptr as well. I did it that way in my previous changes. You know, my intention is to avoid additional changes to the client code, e.g. model code in fairseq and C10D.

@jgong5 data_ptr is in the storage and opaque tensorimpl does not have storage, so I don't think we will turn the access to data_ptr back on.
How did you plan to share the data_ptr of mkldnn tensors?

How did you plan to share the data_ptr of mkldnn tensors?

@bddppq I planned to expose MKL-DNN tensor in a way to support element-wise operation on its buffer. Take C10D as an example, I hoped to pass an MKL-DNN tensor to C10D directly without extra format conversion. Inside C10D, it uses data_ptr and numel to get the buffer for communication:
https://github.com/pytorch/pytorch/blob/master/torch/lib/c10d/ProcessGroupGloo.cpp#L1417

@jgong5 Hmm but that piece of code in c10 assume the underlying storage is contiguous:

  auto ptr = tensor.data_ptr();
  auto size = tensor.numel() * tensor.element_size();

  // Construct unbound buffer.
  auto& context = contexts_[0];
  auto buf = context->createUnboundBuffer(ptr, size);
  buf->send(dstRank, utag);

I don't think that holds for opaque tensorimpl. A better option would be define an interface for remote serialization for tensors and update c10d to use that.

@jgong5 Hmm but that piece of code in c10 assume the underlying storage is contiguous:
I don't think that holds for opaque tensorimpl. A better option would be define an interface for remote serialization for tensors and update c10d to use that.

@bddppq Yes, I realized that point but this is benign for element-wise operation of MKL-DNN tensors as long as they have same format. I agree with you that abstracting out an interface for opaque tensor would be more general.

But how can we handle the model code using data_ptr to check tensor equality?
https://github.com/pytorch/fairseq/blob/master/fairseq/modules/multihead_attention.py#L75

I think we could enable data_ptr in the future. To support that I'd want to figure out which places are currently calling data_ptr() now -- essentially you want two different APIs for "opaque data pointer" (e.g. I just want to copy over this buffer) and "data pointer where indexing is valid." At the python level we can probably just use the same API, but some sanity at the C++ API would be good.

But to @bddppq's point, data_point, numel, contiguous, etc. aren't sufficient for writing a serializer. Because it already doesn't work for sparse. CC @VitalyFedyunin.

Is there a specific use case you have code to support if data_ptr were enabled now?