[GitHub] zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo request for mkldnn operators
zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo
request for mkldnn operators
URL: https://github.com/apache/incubator-mxnet/pull/11129#discussion_r195511194
##
File path: src/common/exec_utils.h
##
@@ -98,10 +98,13 @@ inline bool SetupDefaultBlobsOut(const
std::vector& src,
is_default = nd.IsDefaultData();
#endif
if (!is_default) {
- NDArray temp = bufs != nullptr ? bufs->at(i) : NDArray(nd.shape(),
nd.ctx(),
- true, nd.dtype());
#if MXNET_USE_MKLDNN == 1
+NDArray temp = bufs != nullptr ? bufs->at(i) : nd.IsMKLDNNData() ?
+nd.Reorder2Default() : NDArray(nd.shape(), nd.ctx(), true,
nd.dtype());
Review comment:
the logic here looks very complex. i guess you might need to check
`IsMKLDNNData` first before checking `bufs`.
Also, what happens if nd is a default dense array and req is kAddTo? should
we use nd directly? Should you check kAddTo more explicitly?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo request for mkldnn operators
zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo
request for mkldnn operators
URL: https://github.com/apache/incubator-mxnet/pull/11129#discussion_r195148473
##
File path: src/common/exec_utils.h
##
@@ -98,10 +98,13 @@ inline bool SetupDefaultBlobsOut(const
std::vector& src,
is_default = nd.IsDefaultData();
#endif
if (!is_default) {
- NDArray temp = bufs != nullptr ? bufs->at(i) : NDArray(nd.shape(),
nd.ctx(),
- true, nd.dtype());
#if MXNET_USE_MKLDNN == 1
+NDArray temp = bufs != nullptr ? bufs->at(i) : nd.IsMKLDNNData() ?
+nd.Reorder2Default() : NDArray(nd.shape(), nd.ctx(), true,
nd.dtype());
Review comment:
when `MXNET_USE_MKLDNN == 1`, isn't `is_default` the same as
`nd.IsMKLDNNData()`?
why do you still need to check `nd.IsMKLDNNData()` here?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo request for mkldnn operators
zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo
request for mkldnn operators
URL: https://github.com/apache/incubator-mxnet/pull/11129#discussion_r195152303
##
File path: src/operator/nn/mkldnn/mkldnn_base.cc
##
@@ -128,12 +201,18 @@ void CommitOutput(const NDArray , const
mkldnn_output_t ) {
if (res.first == CopyBack) {
const_cast(arr).CopyFrom(*res.second);
} else if (res.first == AddBack) {
+auto res_memory = res.second;
+auto target_pd = arr.GetMKLDNNData()->get_primitive_desc();
auto mem = arr.GetMKLDNNData(res.second->get_primitive_desc());
-CHECK(mem != nullptr);
+if (mem == nullptr) {
+ auto tmp_memory = TmpMemMgr::Get()->Alloc(target_pd);
+ CopyMKLDNNMem(*res_memory, tmp_memory);
Review comment:
I see. When there is a format mismatch between `arr` and `res`, you use the
format of `arr`. In this case, you may remove line 214 and 216 and have the
sum written to `mem` directly. WriteInplace works in MKLDNN as long as the
inputs and outputs have the same shape and format.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo request for mkldnn operators
zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo
request for mkldnn operators
URL: https://github.com/apache/incubator-mxnet/pull/11129#discussion_r194900964
##
File path: tests/cpp/operator/mkldnn.cc
##
@@ -571,9 +613,8 @@ void VerifyActResult(const std::vector
_arrs, const NDArray )
mshadow::default_real_t *d1 =
static_cast(blob1.dptr_);
mshadow::default_real_t *d2 =
static_cast(blob2.dptr_);
EXPECT_EQ(tmp1.shape().Size(), tmp2.shape().Size());
- for (size_t i = 0; i < tmp1.shape().Size(); i++) {
-EXPECT_EQ(d1[i], std::fmax(d2[i], 0));
- }
+ for (size_t i = 0; i < tmp1.shape().Size(); i++)
+ASSERT_EQ(d2[i], std::fmax(d1[i], 0));
Review comment:
ASSERT_EQ is defined by mxnet. We should use EXPECT_EQ in the unit test
because we want to see all failures.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo request for mkldnn operators
zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo
request for mkldnn operators
URL: https://github.com/apache/incubator-mxnet/pull/11129#discussion_r194812866
##
File path: src/operator/nn/mkldnn/mkldnn_base.cc
##
@@ -304,18 +383,26 @@ void FallBackCompute(FCompute fn, const nnvm::NodeAttrs
,
MKLDNNStream::Get()->Submit();
std::vector out_blobs(outputs.size());
+ std::vector temp_src, temp_dst;
for (size_t i = 0; i < out_blobs.size(); i++) {
NDArray output = outputs[i];
// ensure output does not use mkldnn mem.
// for inplace, we already converted & copied input above.
-if ((req[i] == kWriteTo) || (req[i] == kWriteInplace))
+if ((req[i] == kWriteTo) || (req[i] == kWriteInplace)) {
const_cast(output).InvalidateMKLDNNData();
-else if (req[i] == kAddTo)
- output = outputs[i].Reorder2Default();
+} else if (req[0] == kAddTo && output.IsMKLDNNData()) {
+ NDArray temp = outputs[0].Reorder2Default();
Review comment:
it should be outputs[i]
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo request for mkldnn operators
zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo
request for mkldnn operators
URL: https://github.com/apache/incubator-mxnet/pull/11129#discussion_r194810836
##
File path: src/operator/nn/mkldnn/mkldnn_base.cc
##
@@ -128,12 +201,18 @@ void CommitOutput(const NDArray , const
mkldnn_output_t ) {
if (res.first == CopyBack) {
const_cast(arr).CopyFrom(*res.second);
} else if (res.first == AddBack) {
+auto res_memory = res.second;
+auto target_pd = arr.GetMKLDNNData()->get_primitive_desc();
auto mem = arr.GetMKLDNNData(res.second->get_primitive_desc());
-CHECK(mem != nullptr);
+if (mem == nullptr) {
+ auto tmp_memory = TmpMemMgr::Get()->Alloc(target_pd);
+ CopyMKLDNNMem(*res_memory, tmp_memory);
Review comment:
I think you can just call GetMKLDNNDataReorder.
@pengzhao-intel tries to fix this bug, but doesn't have a test for his fix.
https://github.com/apache/incubator-mxnet/pull/11095
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo request for mkldnn operators
zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo
request for mkldnn operators
URL: https://github.com/apache/incubator-mxnet/pull/11129#discussion_r194811728
##
File path: src/operator/nn/mkldnn/mkldnn_base.cc
##
@@ -304,18 +383,26 @@ void FallBackCompute(FCompute fn, const nnvm::NodeAttrs
,
MKLDNNStream::Get()->Submit();
std::vector out_blobs(outputs.size());
+ std::vector temp_src, temp_dst;
for (size_t i = 0; i < out_blobs.size(); i++) {
NDArray output = outputs[i];
// ensure output does not use mkldnn mem.
// for inplace, we already converted & copied input above.
-if ((req[i] == kWriteTo) || (req[i] == kWriteInplace))
+if ((req[i] == kWriteTo) || (req[i] == kWriteInplace)) {
const_cast(output).InvalidateMKLDNNData();
-else if (req[i] == kAddTo)
- output = outputs[i].Reorder2Default();
+} else if (req[0] == kAddTo && output.IsMKLDNNData()) {
Review comment:
I wonder if it's necessary that in the case of fallback, the output is still
an MKLDNN array?
I know the unit test can trigger the problem.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo request for mkldnn operators
zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo
request for mkldnn operators
URL: https://github.com/apache/incubator-mxnet/pull/11129#discussion_r194813020
##
File path: src/operator/nn/mkldnn/mkldnn_base.cc
##
@@ -304,18 +383,26 @@ void FallBackCompute(FCompute fn, const nnvm::NodeAttrs
,
MKLDNNStream::Get()->Submit();
std::vector out_blobs(outputs.size());
+ std::vector temp_src, temp_dst;
for (size_t i = 0; i < out_blobs.size(); i++) {
NDArray output = outputs[i];
// ensure output does not use mkldnn mem.
// for inplace, we already converted & copied input above.
-if ((req[i] == kWriteTo) || (req[i] == kWriteInplace))
+if ((req[i] == kWriteTo) || (req[i] == kWriteInplace)) {
const_cast(output).InvalidateMKLDNNData();
-else if (req[i] == kAddTo)
- output = outputs[i].Reorder2Default();
+} else if (req[0] == kAddTo && output.IsMKLDNNData()) {
+ NDArray temp = outputs[0].Reorder2Default();
+ temp_src.emplace_back(temp);
+ temp_dst.emplace_back(outputs[0]);
+ output = temp;
+}
CHECK(output.IsDefaultData());
out_blobs[i] = output.data();
}
+
fn(attrs, ctx, in_blobs, req, out_blobs);
+ if (req[0] == kAddTo && outputs[0].IsMKLDNNData())
Review comment:
why is the index always 0?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo request for mkldnn operators
zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo
request for mkldnn operators
URL: https://github.com/apache/incubator-mxnet/pull/11129#discussion_r194813504
##
File path: src/operator/nn/mkldnn/mkldnn_sum.cc
##
@@ -58,46 +58,54 @@ void MKLDNNSumForward(const nnvm::NodeAttrs& attrs, const
OpContext ,
std::vector in_pds(inputs.size());
std::vector scales(inputs.size(), 1);
in_prims.reserve(inputs.size());
+
+ auto out_mem = out_data.GetMKLDNNData();
+ auto target_pd = out_mem->get_primitive_desc();
bool pd_same = true;
std::vector in_bufs(inputs.size());
for (size_t i = 0; i < inputs.size(); i++) {
const mkldnn::memory *in_mem;
if (inputs[i].IsMKLDNNData() && inputs[i].IsView()) {
in_bufs[i] = inputs[i].Reorder2Default();
- in_mem = in_bufs[i].GetMKLDNNData();
+ in_mem = in_bufs[i].GetMKLDNNData(target_pd);
} else {
- in_mem = inputs[i].GetMKLDNNData();
+ in_mem = inputs[i].GetMKLDNNData(target_pd);
+}
+if (in_mem == nullptr) {
+ auto tmp_memory = TmpMemMgr::Get()->Alloc(target_pd);
+ auto input_memory = inputs[i].GetMKLDNNData();
+ CopyMKLDNNMem(*input_memory, tmp_memory);
Review comment:
I think you can just call GetMKLDNNDataReorder
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo request for mkldnn operators
zheng-da commented on a change in pull request #11129: [MXNET-497]Test kAddTo
request for mkldnn operators
URL: https://github.com/apache/incubator-mxnet/pull/11129#discussion_r194808938
##
File path: src/common/exec_utils.h
##
@@ -98,10 +98,13 @@ inline bool SetupDefaultBlobsOut(const
std::vector& src,
is_default = nd.IsDefaultData();
#endif
if (!is_default) {
- NDArray temp = bufs != nullptr ? bufs->at(i) : NDArray(nd.shape(),
nd.ctx(),
- true, nd.dtype());
#if MXNET_USE_MKLDNN == 1
+NDArray temp = bufs != nullptr ? bufs->at(i) : nd.IsMKLDNNData() ?
+nd.Reorder2Default() : NDArray(nd.shape(), nd.ctx(), true,
nd.dtype());
CHECK(temp.IsDefaultData());
+#else
+ NDArray temp = bufs != nullptr ? bufs->at(i) : NDArray(nd.shape(),
nd.ctx(),
+ true, nd.dtype());
Review comment:
sparse arrays doesn't have kAddTo? @eric-haibin-lin
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
