For the comment, the original 'NCHW format' does not stand now.
BTW, conv with dilation and ndim=3/4/5 (aka. 1D, 2D, 3D) all supported by mkldnn. But we can do it with a separate PR.

These checks are for the conv on CPU tensor. Probably we just keep what has been done on the original path and put all future opts in the new MKL-DNN tensor path?

Oh, that is for CPU tensor. Then forget what I said :-)

what's the story with the other checks? We can't use CPU tensor / MKLDNN algo if it's dilated / transposed / or the input dimension is wrong, but we can if it's an MKLDNN tensor?

And where's the check if it's not conv2d -- what happens if you try to call one of the other algos with an MKLDNN tensor?

How about exploring the spaces of kernel_size, stride and padding a little bit? From C2, we used the setting like below via hypothesis attributes:

    @given(stride=st.integers(1, 3),
           pad=st.integers(0, 3),
           kernel=st.integers(3, 5),
           size=st.integers(8, 10),
           input_channels=st.integers(1, 3),
           output_channels=st.integers(1, 5),
           batch_size=st.integers(1, 3),
           use_bias=st.booleans(),
           training_mode=st.booleans(),
           group=st.integers(1, 2),

yeah, we should probably start introducing hypothesis for cases like this. @gchanan - thoughts?

I don't have any objections to that, although it would be great to start with the existing tests so it doesn't live in its own little world.

shall we provide MKLDNN tensor with contiguous()?
let is_contiguous() return true will do the job? In case we have a MKLDNN tensor, it should always be contiguous.

The question is how we can define sound semantics of contiguous for opaque tensors. It does not make sense to have is_contiguous return true when contiguous is not well defined.

you can check TensorTypeId of course, i did not see any discomfortable to let MkldnnCPUTensorId return true for tensor.is_contiguous().

I'm a little worried of making contiguous work on MKLDNN as the semantics are not well-defined. Maybe add a wrapper function contiguous_or_mkldnn and call it only in this implementation?

@dzhulgakov Indeed the semantics indicates that mkldnn blocked layout should be non-contiguous while at the same time mkldnn function calls just need to know that whether the buffer is a physical contiguous chunk of memory (which is always yes for mkldnn tensor).

In case we don't inherit contiguous() for mkldnn tensor. Another simple method will be move all input.contiguous(), weight.contiguous(), bias.contiguous() from _convolution() to each underlying device implementations: a.k.a cudnn_convolution() , mkldnn_convolution() etc.
In this case, at the upper level from _convolution() will be unified (look cleaner) and inside mkldnn_convolution we are free to do something like:

// at::native::mkldnn_convolution(const Tensor& input_r, const Tensor& weight_r, ...)
auto input = is_input_mkldnn ? input_r : input_r.contiguous()
auto weight = is_weight_mkldnn ? weight_r : weight_r.contiguous()

and we can do this kind of check ONLY inside mkldnn function calls, so mkldnn tensor don't have to inherit contiguous()

also once we have contiguous(), we don't have to do if-else here depending on input_is_mkldnn, just one call to at::mkldnn_convolution is needed

nit: maybe keep weight_sizes as ArrayRef and have a separate vector for sizes in mkldnn case and then point ArrayRef to that vector?

Does ArrayRef extend the lifetime of a vector? (If not then we still can not avoid the creation of a vector?)

ArrayRef doesn't extend the lifetime of the vector.

I'm a little worried of making contiguous work on MKLDNN as the semantics are not well-defined. Maybe add a wrapper function contiguous_or_mkldnn and call it only in this implementation?

yeah, we should probably start introducing hypothesis for cases like this. @gchanan - thoughts?

@mingfeima - so in general - is this reordering for inference only? if we do it in training, we'd need to make sure that all optimizers can handle different layout/dimensions and even auxiliary weights like for Adam work well.

can we just do the reorder on the fly in the function? I'm unclear if the reorder is expensive -- it looks pretty simple, but not sure how it's actually implemented.

@dzhulgakov Pre-reordering weights is still useful for training even though perf gain might not be that obvious, depending on the workloads. It is possible to leave existing optimizers intact to support the opaque tensor as long as it supports all the ops and factory functions used by the optimizers.

reorder on the fly (repeatedly) will cause 20% slower in conv (and 15% slower in the whole resnext).

is that slower than non-mkldnn or slower than mkldnn with optimization?

What I'm trying to get at: if the "on the fly" packing is still faster than the dense path, then it makes sense to expose this op independent of pre-packing. But if it's not, we need to tie this into our greater weight pre-packing story.

can you just call _apply?

_apply doesn't pass the module itself to the callback, but we need it to check whether it's conv (and if yes we need to do extra conversion).

If you just want to match children modules - there's .apply

can we just do the reorder on the fly in the function? I'm unclear if the reorder is expensive -- it looks pretty simple, but not sure how it's actually implemented.

I don't have any objections to that, although it would be great to start with the existing tests so it doesn't live in its own little world.

what's the story with the other checks? We can't use CPU tensor / MKLDNN algo if it's dilated / transposed / or the input dimension is wrong, but we can if it's an MKLDNN tensor?

And where's the check if it's not conv2d -- what happens if you try to call one of the other algos with an MKLDNN tensor?

it assumes that all ops work on mkldnn which might not be the case. Maybe we should have two versions: conservative that matches only some modules and full-on that handles all params

Hmm supporting partial conversion is hard I think, you will need to add to_mkldnn and to_dense at the boundaries.

If you just want to match children modules - there's .apply

@mingfeima - just curious - does mkldnn tensor for conv stored somewhere internally the original semantic sizes? I'm just worried that we hard-code logic here that might change later as mkldnn does potentially more optimizations

Nope. mkldnn tensor stores only mkldnn::memory::dims.

There might be some risks here when generating output with new_with_itensor_mkldnn in case mkldnn::memory::dims is not align with at::sizes(). Since the output aten tensor would have sizes equals to mkldnn dims.

So far for CNN modules, i didn't see any problem, since output are all 4D tensors.
For RNN module, it's going to be an issue, since mkldnn dims and aten sizes are not aligned. (mkldnn rnn weight has 5d dims but aten weight has 2d size). Anyway, perhaps i can wrap this up inside ideep so that you don't have to worry about this.

Thanks for writing this comment!

By the time -> At the time of writing this note,

we will probably need to move this call elsewhere in order to define "prepack for inference" interface in the future. Otherwise it's hard to serialize the mkldnn including the particular layout in the file. But it can be done in a separate PR when we get to more API pieces

we will probably need to move this call elsewhere in order to define "prepack for inference" interface in the future. Otherwise it's hard to serialize the mkldnn including the particular layout in the file.

The pre-packing is also useful for training since it avoids extra reorders as well. In order to serialize the weights properly, can we always to_dense when the module is serialized?

For training more work would be needed - for example handling mkldnn tensors in optimizer

What I meant about inference is that we'd need a mechanism to bundle instructions to pack the weights after deserialization to mkldnn format (since as you said they'd be stored in as regular to_dense tensors). We have a prototype mechanism for it (https://github.com/pytorch/pytorch/blob/master/torch/jit/quantized.py#L34) but it needs to be improved. Most likely, we'd need to swap out Conv2d module with special MkldnnConv2d module that handles packing after loading

#23042