refine the mkldnn logic #4953
refine the mkldnn logic #4953
Conversation
and change the size of test to make unit test faster refine comment and log of mkldnnlayer
tensor-tang
force-pushed
the
merge_grad_gtest
branch
from
004d812 to
4f41eaf
Compare
There was a problem hiding this comment.
CHECK的部分需要重新梳理下（可以在下一个PR改，这个PR内容有点多）：
- 一部分重复的，可以定义宏来简化。
- 每个CHECK里面，只检查一个内容，现在有大量的&&在CHECK中。如果这一行挂了，不能定位到底是哪个出错。
CHECK(outVal_ != nullptr)写成CHECK(outVal_ )即可。
再次希望PR能少一点，便于理解、review和尽快merge。比如这里的第四个commit（refine the gtest log info and vlog order, and change the size of test to make unit test faster refine comment and log of mkldnnlaye），和其他几个无关，建议拆出来。
| bias, biases_->getWGrad(), wgtPD->diff_bias_primitive_desc()); | ||
| CHECK(bias && biasVal_ && | ||
| bias->getPrimitiveDesc() == biasVal_->getPrimitiveDesc()) | ||
| << "primitive desc of bias grad should equal the bias value"; |
There was a problem hiding this comment.
313-315行，321-323行，这些check也能抽出一个函数么？
看MKLDNN相关的代码，CHECK很多，可以写个宏之类来定义下不同的CHECK么，这样能省去不少代码。
可以参考PADDLE_ENFORCE
There was a problem hiding this comment.
嗯，可以考虑下，以后来看看能不抽象出一个宏定义来。
| if (biases_ == nullptr || biases_->getW() == nullptr) { | ||
| return; | ||
| } | ||
| resetWithMatrix(bias, biases_->getW(), pd->bias_primitive_desc()); |
There was a problem hiding this comment.
这里写成下面这种会比较简短：
if (biases_ && biases_->getW() ) {
resetWithMatrix(bias, biases_->getW(), pd->bias_primitive_desc());
}
There was a problem hiding this comment.
嗯，可以的，我可以后面一起改掉，bias的问题也需要在整洁下。
|
|
||
| resetOutValue(pd, out); | ||
| bias = nullptr; |
There was a problem hiding this comment.
从resetWithMatrix实现来看，213行可以去掉。
There was a problem hiding this comment.
ok，可以按照if else来，默认最好清个零。
| if (dataPD == nullptr) { | ||
| return; | ||
| } | ||
| resetInGrad(in, dataPD->diff_src_primitive_desc()); | ||
| resetWgtValBwdData(dataPD, wgtValBwdData_); |
There was a problem hiding this comment.
326行-330行，写成：
if (dataPD) {
resetInGrad(in, dataPD->diff_src_primitive_desc());
resetWgtValBwdData(dataPD, wgtValBwdData_);
}
There was a problem hiding this comment.
我觉得现在原来那样写好一点，没有就return。
逻辑也清楚点。并且缩进也少点。
如果觉得用nullptr不好，可以写为if(!dataPD)。或者之后专门留PR来整理这些。
| @@ -431,99 +350,13 @@ void MKLDNNConvLayer::resetBwdPipeline( | |||
| if (dataPD == nullptr) { | |||
| return; | |||
| } | |||
There was a problem hiding this comment.
同上，写成：
if (dataPD) {
XXX;
}
| << "only support input is MKLDNN layer or only have one output layer"; | ||
| // when input is a mkldnn branch node, | ||
| // this layer will save input grad to a internal buffer, | ||
| // and the mkldnn input layer will merge them to actual prev->output_.grad |
There was a problem hiding this comment.
the mkldnn input layer will merge them，merge是在input里面做的？
There was a problem hiding this comment.
嗯，在resetInGrad里面。
| Argument& arg = input->getOutput(this->getName()); | ||
| arg.grad = std::dynamic_pointer_cast<Matrix>(in); | ||
| CHECK(inVal_ != nullptr && inVal_->getPrimitiveDesc() == intPD) | ||
| << "should have internal input value and primitive desc must equal"; |
There was a problem hiding this comment.
这里写成
CHECK(inVal_);
CHECK_EQ(inVal_->getPrimitiveDesc(), intPD) << "internal input value and primitive desc must equal";
一行check一个值，比较好。不然这里挂了，不知道是inval空导致的，还是不等于导致的。
paddle/gserver/layers/MKLDNNLayer.h
Outdated
| /// The output_.value and output_.grad always save the external data, | ||
| /// when mixed with cpu device. | ||
| /// When all layers are mkldnn layers, they could save internal data. | ||
| /// below MKLDNNMatrix buffers are all internal buffers |
There was a problem hiding this comment.
- 每一行开头要大写，Value，Each都要大写。
- is always nchw of nc，这里是of还是or？
- 67行和68行中间要空一行。
- 61-67行可以用/* */来注释。
paddle/gserver/layers/MKLDNNLayer.h
Outdated
| * reset output grad from internal primitive desc. | ||
| * merge grad if necessary. | ||
| * reset both internal and external buffer and create reorder if necessary. | ||
| * note: about merge grad, when this layer has serval outputs, |
paddle/gserver/layers/MKLDNNLayer.h
Outdated
|
|
||
| /** | ||
| * Print the mkldnn memory format flow of value | ||
| * reset the merge grad primitive if necessary. | ||
| * note: do not support the grads are mixed with cpu device, |
There was a problem hiding this comment.
do not support the grads mixed with cpu device
去掉are，一个句子里面只有一个动词。
refine comments and bias fix typo and todo
|
Done. |
MKLDNNLayer子类的resetInValue等类似的函数。在父类实现，子类只负责调用。MKLDNNMatrix::create的接口，把MatrixPtr放到后面，作为不是必须参数。MKLNNLayer.cpp文件，把必要的函数实现放在了cpp文件中。