Enable mkldnn_fc for general format #3337

tensor-tang · 2017-08-08T13:56:58Z

enable mkldnn_fc and gtest for mkldnn
fix #3383

wangkuiyi

Thanks to @tensor-tang for this PR!

I have some comments about naming and a question about the newly added flags.

wangkuiyi · 2017-08-08T16:26:44Z

paddle/gserver/layers/MkldnnBase.h

Mkldnn => MKLDNN 英语缩写应该大写

wangkuiyi · 2017-08-08T16:26:56Z

paddle/gserver/layers/MkldnnBase.h

Cpu => CPU 英语缩写应该大写

CpuEngine这个直接写成一个全局函数这样是否就可以了？

mkldnn::engine& getCpuEngine() { static mkldnn::engine cpuEngine(mkldnn::engine::cpu, 0); return cpuEngine; }

这里参考是这里
后续可能还会考虑FPGA的engine。

Cpu => CPU 英语缩写应该大写

@wangkuiyi 还是我之前的疑虑。
因为我参照的paddle原有的命名规则，比如CpuMatrix, GpuMatrix, 还包括文件名的例子CudnnPoolLayer.cpp等等。
这些的命名规则我都没有问题，可以修改的，只需要告诉我最终采用哪种即可，我的目的还是只要符合paddle的需求就好。

Thanks~

后面类似的我就不一一comment了吧。

我很理解 @tensor-tang 。这是我们之前没有说明白的地方。Paddle之前和Caffe2的代码里都采用了“非常常见的，以至于形成了新词的英语缩写不再当做缩写，也就不用完全大写”的规则。但是我们实际上很难执行，因为不好判断哪些缩写已经常用到可以作为新词。比如FPGA在做FPGA的人眼里足够常见了，但是很多deep learning researchers甚至都没有听说过。为此，我们的新代码库里，所有的英语缩写都保持大写。

好的，那我会统一按照最新标准执行。Thanks~

Done

wangkuiyi · 2017-08-08T16:27:08Z

paddle/gserver/layers/MkldnnFcLayer.cpp

MKLDNNFcLayer.h

wangkuiyi · 2017-08-08T16:27:31Z

paddle/gserver/layers/MkldnnFcLayer.h

MKLDNNLayer.h

wangkuiyi · 2017-08-08T16:28:30Z

paddle/gserver/layers/MkldnnLayer.h

MKLDNNLayer

wangkuiyi · 2017-08-08T16:30:13Z

paddle/gserver/layers/MkldnnLayer.h

We don't need ; here.

wangkuiyi · 2017-08-08T16:31:15Z

paddle/gserver/tests/MkldnnTester.cpp

MKLDNNTester.h

wangkuiyi · 2017-08-08T16:34:40Z

paddle/gserver/layers/MkldnnFcLayer.cpp

I don't get it -- why we need this flags. If we don't want to convert weights from Paddle, how about we just do not call this function?

这里是一个支持。
因为mkldnn训练出来的weight，数据的排列方式与paddle原有的不一样。
比如，原先我们用paddle的cpu已经训练出来了一组weight，现在想要用mkldnn的engine去加速inference，就需要use_mkldnn_wgt=False，这个函数就很有必要了。

在当前FC的例子里面，仅仅是一个转置的关系，还比较简单，但是后面别的layer就会比较复杂。
采用这个flag是给用户一个选择，输入的weight是不是用的mkldnn格式weight，如果是的，就不用这步转换了。

采用这个flag是给用户一个选择，输入的weight是不是用的mkldnn格式weight

这个对用户来说要求太高了。这个能做成自动透明的么？

同意。
所以，我本来有个额外的计划，目前暂时采用这个flag，最终我也希望最后mkldnn在保存weight的时候（也不好在每个iteration结束后都转换，这样太浪费了）自动调用convert函数，存为cpu的格式，这样这个flag就可以去掉了。
但是我遇到一个问题，调用save的时候用的都是parameter的接口，这个函数要么存在mkldnnlayer，要么以后希望放在MkldnnMatrix里面，没有地方加这样一个逻辑。所以，暂时用这个flag来兼顾。

wangkuiyi · 2017-08-08T16:34:56Z

paddle/gserver/layers/MkldnnFcLayer.cpp

cvtWgtFromPaddle => ConvertWeightsFromPaddle

wangkuiyi · 2017-08-08T16:37:12Z

paddle/utils/Flags.cpp

I don't get it -- why would we need these flags. We had a discussion that we shouldn't add new flags, because they are actually global variables. For this specific PR, are these flags necessary?

这里是像 use_gpu一样，引入 use_mkldnn是为了做layer间的选择，是选择用mkldnn还是普通的cpu layer。如果不采用这个flag的话，mkldnnfc就需要单独用api来使用了，不是很通用。

use_mkldnn_wgt这个flag就像前面说的一样，是为了更好的支持paddle原有的cpu weight，并且，如果本来就是用mkldnn进行pretrain的话，也可以省掉不必要的转换。

We had a discussion that we shouldn't add new flags

这一点我记得我们是align过的，当时说的是可以添加flag的。

tensor-tang · 2017-08-09T02:36:53Z

Thanks @wangkuiyi .

关于整体命名规则，我有点疑问，因为paddle原有的规则看起来，好像只有首字母大写了：

Cpu => CPU 英语缩写应该大写

因为我参考的是CpuMatrix和CpuMatrixPtr

Mkldnn => MKLDNN 英语缩写应该大写
MKLDNNFcLayer.h
MKLDNNTester.h

参考的是CudnnBatchNormLayer

所以整体才是现在这样。

luotao1

use_mkldnn_wgt这个flag就像前面说的一样，是为了更好的支持paddle原有的cpu weight，并且，如果本来就是用mkldnn进行pretrain的话，也可以省掉不必要的转换。

这个flag在这个PR中可以先去掉么？等之后考虑pretrain的时候，再加入。

mkldnnLayer中有FC相关的代码，这部分能挪到paddle/mkl里面，类似paddle/cuda的做法？父类中不应该有子类的信息。
关于单测，可以考虑复用paddle的testLayerGrad函数和test_CompareTwoNetworks么？

luotao1 · 2017-08-09T06:17:51Z

paddle/gserver/layers/MkldnnBase.h

+  DNN_SIZES,
+  DNN_FMTS,
+  DNN_ALL,
+} DNN_LOG_LEVEL;


21-27行每行都需要注释。

DNN_BASE和DNN_TESTS为什么都是1呢

这里的DNN需要换成MKLDNN么？

21-27行每行都需要注释。
ok

DNN_BASE和DNN_TESTS为什么都是1呢
这是是自己定的，为了方便区分名字，但是实际值没必要区分

这里的DNN需要换成MKLDNN么？

是可以改成DNN => MKLDNN的，我是考虑名字改完后名字太长了~

这些都是VLOG的时候用的

为了和cudnn做区分，所以要改成MKLDNN。
如果两个都是1，直接用DNN_BASE就行了，不用再区分。

这里会改为MKLDNN_BASE。
还是要区分下，用的场合不一样，一个用做基本信息，一个仅仅用在test里面。只是值一样罢了，如果以后觉得值想不一样，改起来也很方便。

luotao1 · 2017-08-09T06:20:47Z

paddle/gserver/layers/MkldnnBase.h

+  /**
+   * @brief Submit stream
+   * @param prims The primitives vector
+   *        block Waiting for the stream to complete


@param block

luotao1 · 2017-08-09T06:22:34Z

paddle/gserver/layers/MkldnnBase.h

+    }
+    // TODO(TJ): change me when mkldnn have method to reset this state
+    stream_.reset(new mkldnn::stream(mkldnn::stream::kind::eager));
+    // stream_.reset(new mkldnn::stream(mkldnn::stream::kind::lazy));


88行注释的代码去掉。

这个先留着吧，不建议去掉吧，因为是一种选择，现在不好说哪个性能好。加在下面对比用的。

如果要留的话，也应该留在87行前面，和86行的注释连在一起。

好的，我挪一下位置。thx

luotao1 · 2017-08-09T06:25:48Z

paddle/gserver/layers/MkldnnLayer.h

+   */
+  virtual void convertWeightsToPaddle() {}
+
+  void resetForwardFC(int bs,


resetForwardFC和resetBackwardFC要加注释

luotao1 · 2017-08-09T06:26:58Z

paddle/gserver/layers/MkldnnLayer.h

+
+  // TODO(TJ): change below memory as MkldnnMatrixPtr type
+  // input == bottom, output == top
+  // value == data, grad == diff


53-54的注释是描述哪段呢？

就是下面的命名的解释。

下面的55-62行没有出现input、bottom、output、top、value、data、grad、diff的任何一个单词。

本来的目的是一个提示作用，因为比如bwd时的真正“input数据” 实际是这里的outGrad，为了起到一个提示作用，所以才加注释用bottom和top区分。
如果不希望看到，就删掉吧。

luotao1 · 2017-08-09T07:13:56Z

paddle/gserver/layers/MkldnnLayer.cpp

+
+#include "MkldnnLayer.h"
+
+using mem = mkldnn::memory;  // NOLINT


mem->mklmem

这个仅仅是这个cpp里面，目的就是为了简写，所以没必要专门改为mklmem了吧？

需要修改，mklmem比mem更直观。

ok，会调整。但是我觉得如果要改，mklmem也不好吧。采用use namespace吧，会把先都改为memory。

luotao1 · 2017-08-09T07:14:45Z

paddle/gserver/layers/MkldnnLayer.cpp

+                       const ParameterMap& parameterMap) {
+  if (!Layer::init(layerMap, parameterMap)) {
+    return false;
+  }


直接调用Layer::init(layerMap, parameterMap即可，不用条件判断

有返回值的，需要判断的。

luotao1 · 2017-08-09T07:20:00Z

paddle/gserver/layers/MkldnnLayer.cpp

+  outGrad_->set_data_handle(topDiff);
+
+  stream_->submit(pipelineBwd_);
+}


mkldnnLayer.cpp中存在很多FC相关的代码，这些应该和cuda一样，放在paddle/mkl/src里面。mkldnnLayer.cpp只是一个基类，不能放继承类的信息。
@hedaoyuan

这里是一个整体，fc子类只是调用接口，父类提供了一系列的方法，不是想代表继承类的信息，以后也会包括其它，包括conv等。

因为这里父类为每个子类提供了不同的方法，但这些方法应该是在子类中实现。父类只是提供一个通用的方法，具体实现应该在子类中完成。

我也有疑问，问什么mkldnnForwardFC, mkldnnBackwardFC放在MkldnnLayer里面，而不是放在MkldnnFcLayer里面。

那么我来调整下，想办法放到fc中。
@luotao1 @hedaoyuan

luotao1 · 2017-08-09T07:32:42Z

paddle/gserver/tests/test_Mkldnn.cpp

+    TestConfig ref = cfg;
+    ref.layerConfig.set_type(compareTypes[1]);
+    for (auto bs : {pm.bs, 1}) {
+      tester.run(cfg, ref, bs, pm.ih, pm.iw);


这里不能直接调用testLayerGrad函数么？

这里check的不是grad的波动等信息，并且check的点也不一样。这里着重比较在不同size下的与cpu各方面结果一致。
用c++ code更方便，也更好维护。如果用test_compareNet会更麻烦，其实这里等于说每个size的比较都需要写两个配置文件，这里用gtest只需要一行。

luotao1 · 2017-08-09T07:35:41Z

paddle/gserver/tests/MkldnnTester.cpp

+
+  // reset configs and layer names
+  configs_[DNN] = dnn;
+  configs_[REF] = ref;


mkldnnTest是用来比较两个layer间是否无diff。
能不能配置两个简单的网络（只有data+fc），直接调用paddle的test_CompareTwoNetworks?这样代码少很多。

目前不好直接用，因为mkldnn的中间输出是内部格式的weight，肯定与cpu的不一样。
并且我建议把mkldnn相关的检查与原有的方法区分开来，方便维护。

后期功能齐全了之后可以考虑用comparenet，目前仍然建议安装每个layer的逐一确认。

tensor-tang

这个flag在这个PR中可以先去掉么？等之后考虑pretrain的时候，再加入。

建议选择加入，因为在gtest的时候需要比较weight，所以会用到。

关于单测，可以考虑复用paddle的testLayerGrad函数和test_CompareTwoNetworks么？

单测不建议使用testLayerGrad函数和test_CompareTwoNetworks。

用一个test_mkldnn搞定所有mkldnn相关的检测，也很方便我后期维护。
这里测试的重点在于对于，不同size的输入输出，都能等于cpu的结果，而不是在layergrad。
这里用一行代码可以解决一个test case，如果用test_CompareTwoNetworks则需要配置两个小网络，更加烦琐。并且现在功能还不齐全，可以等后面layer齐全点，一次比较一个深一点的网络更有意义。

mkldnnLayer中有FC相关的代码，这部分能挪到paddle/mkl里面，类似paddle/cuda的做法？父类中不应该有子类的信息。

像之前说的那样，mkldnn与cudnn既有相似也有不一样。相似的是接口相似，不相似的是使用时mkldnn还需要很多primitive和memory的desc，这些东西还是放在父类最合适。因为要用到一系列的mkldnn 自己的memory，这些留在父类直接访问就好，父类提供每个layer的方法，子类直接用接口就好了。

tensor-tang · 2017-08-09T07:56:13Z

paddle/gserver/layers/MkldnnBase.h

+  DNN_SIZES,
+  DNN_FMTS,
+  DNN_ALL,
+} DNN_LOG_LEVEL;


21-27行每行都需要注释。
ok

DNN_BASE和DNN_TESTS为什么都是1呢
这是是自己定的，为了方便区分名字，但是实际值没必要区分

这里的DNN需要换成MKLDNN么？

是可以改成DNN => MKLDNN的，我是考虑名字改完后名字太长了~

这些都是VLOG的时候用的

tensor-tang · 2017-08-09T07:57:52Z

paddle/gserver/layers/MkldnnBase.h

+  /**
+   * @brief Submit stream
+   * @param prims The primitives vector
+   *        block Waiting for the stream to complete


tensor-tang · 2017-08-09T07:59:25Z

paddle/gserver/layers/MkldnnBase.h

+    }
+    // TODO(TJ): change me when mkldnn have method to reset this state
+    stream_.reset(new mkldnn::stream(mkldnn::stream::kind::eager));
+    // stream_.reset(new mkldnn::stream(mkldnn::stream::kind::lazy));


这个先留着吧，不建议去掉吧，因为是一种选择，现在不好说哪个性能好。加在下面对比用的。

tensor-tang · 2017-08-09T08:00:09Z

paddle/gserver/layers/MkldnnFcLayer.cpp

+                         const ParameterMap& parameterMap) {
+  if (!MkldnnLayer::init(layerMap, parameterMap)) {
+    return false;
+  }


因为有返回值

tensor-tang · 2017-08-09T08:00:19Z

paddle/gserver/layers/MkldnnFcLayer.cpp

+    return false;
+  }
+
+  CHECK_EQ(inputLayers_.size(), 1) << "Only support one input layer yet!";


tensor-tang · 2017-08-09T08:18:50Z

paddle/gserver/layers/MkldnnLayer.h

+   */
+  virtual void convertWeightsToPaddle() {}
+
+  void resetForwardFC(int bs,


tensor-tang · 2017-08-09T08:22:50Z

paddle/gserver/tests/MkldnnTester.cpp

+
+  // reset configs and layer names
+  configs_[DNN] = dnn;
+  configs_[REF] = ref;


目前不好直接用，因为mkldnn的中间输出是内部格式的weight，肯定与cpu的不一样。
并且我建议把mkldnn相关的检查与原有的方法区分开来，方便维护。

后期功能齐全了之后可以考虑用comparenet，目前仍然建议安装每个layer的逐一确认。

tensor-tang · 2017-08-09T08:24:23Z

paddle/gserver/tests/MkldnnTester.cpp

+}
+
+void MkldnnTester::checkBackwardData() {
+  const bool isBN = dnnLayer_->getType() == "mkldnn_batch_norm";


这个留着没关系吧，担心以后考虑忘了。并且这一行不会影响fc的逻辑。他也是属于mkldnn test的一部分，从这个角度也是属于mkldnn tester应该有的。

tensor-tang · 2017-08-09T08:27:02Z

paddle/gserver/tests/test_Mkldnn.cpp

+    TestConfig ref = cfg;
+    ref.layerConfig.set_type(compareTypes[1]);
+    for (auto bs : {pm.bs, 1}) {
+      tester.run(cfg, ref, bs, pm.ih, pm.iw);


这里check的不是grad的波动等信息，并且check的点也不一样。这里着重比较在不同size下的与cpu各方面结果一致。
用c++ code更方便，也更好维护。如果用test_compareNet会更麻烦，其实这里等于说每个size的比较都需要写两个配置文件，这里用gtest只需要一行。

tensor-tang · 2017-08-09T08:28:47Z

python/paddle/trainer/config_parser.py

+            if use_mkldnn and use_mkldnn_wgt:
+                dims = [self.config.size, input_layer.size]
+            else:
+                dims = [input_layer.size, self.config.size]


这里你的逻辑是有问题的，在else部分，你需要多写一段code。我建议还是用现有的逻辑。

luotao1

use_mkldnn_wgt这个flag目前只有在单测中使用，未考虑pretrain的过程。这个flag对用户太不友好了，以后设计pretrain的时候需要重新考虑。因此现在为了单测，可以在mkldnnlayer.h里面创建一个变量 use_mkldnn_wgt，补充get和set这个变量的方法，然后用这个变量来控制单测。
PR应该是相对独立和小的，所以和这个PR无关的信息，比如mkldnn_batch_norm，要去掉。等以后写到这块的时候，再加。

父类提供每个layer的方法，子类直接用接口就好了。

mkldnnlayer是父类，写完后就不应该动了。不能写一个layer，就往父类中加该layer的方法，这样父类会越来越大。因此fc相关的代码还是要挪走。

mkldnn还需要很多primitive和memory的desc

从代码中看只是一些typedef的格式，不一定要放在父类中。

luotao1 · 2017-08-09T09:41:26Z

paddle/gserver/layers/MkldnnBase.h

+  DNN_SIZES,
+  DNN_FMTS,
+  DNN_ALL,
+} DNN_LOG_LEVEL;


为了和cudnn做区分，所以要改成MKLDNN。
如果两个都是1，直接用DNN_BASE就行了，不用再区分。

luotao1 · 2017-08-09T09:42:29Z

paddle/gserver/layers/MkldnnBase.h

+    }
+    // TODO(TJ): change me when mkldnn have method to reset this state
+    stream_.reset(new mkldnn::stream(mkldnn::stream::kind::eager));
+    // stream_.reset(new mkldnn::stream(mkldnn::stream::kind::lazy));


如果要留的话，也应该留在87行前面，和86行的注释连在一起。

luotao1 · 2017-08-09T09:45:14Z

paddle/gserver/layers/MkldnnFcLayer.cpp

+  int batchSize = input.getBatchSize();
+  if (bs_ == batchSize) {
+    return;
+  }


从代码来看，除了 resetOutput(bs_, oc_)和batchsize有关，其他变量的设置都和bs_无关。bs_不变的时候，其他变量也不变么

luotao1 · 2017-08-09T09:48:08Z

paddle/gserver/layers/MkldnnLayer.cpp

+  outGrad_->set_data_handle(topDiff);
+
+  stream_->submit(pipelineBwd_);
+}


因为这里父类为每个子类提供了不同的方法，但这些方法应该是在子类中实现。父类只是提供一个通用的方法，具体实现应该在子类中完成。

luotao1 · 2017-08-09T09:49:17Z

paddle/gserver/layers/MkldnnLayer.cpp

+
+#include "MkldnnLayer.h"
+
+using mem = mkldnn::memory;  // NOLINT


需要修改，mklmem比mem更直观。

luotao1 · 2017-08-09T10:19:41Z

paddle/gserver/tests/MkldnnTester.cpp

+  // Then test FLAGS_use_mkldnn_wgt = true
+  FLAGS_use_mkldnn_wgt = true;
+  // after run once the mkldnn weight has been stored in dnnlayer
+  // then save the weigths and restart again


weigths->weights

luotao1 · 2017-08-09T10:23:46Z

paddle/gserver/tests/test_Mkldnn.cpp

+  int bs;
+  int ic;
+  int oc;
+  int ih, iw;  // oh == ow == 1


int bs; int ic; int ih; int iw; int oc;

按照顺序写。

参考的是这里

luotao1 · 2017-08-09T10:24:52Z

paddle/gserver/tests/test_Mkldnn.cpp

+      {INPUT_DATA,
+       "layer_0",
+       /* size of input layer= */ size_t(pm.ic * pm.ih * pm.iw),
+       /* size of weight= */ size_t(pm.oc * pm.ic * pm.ih * pm.iw)});


weight的size=pm.oc * pm.ic * pm.ih * pm.iw。对么？weight的size应该和output.value一样大。pm.oc * pm.oh *pm.ow

这里weigth的size没有问题。请参考FC的实现原理。

luotao1 · 2017-08-09T10:26:35Z

paddle/gserver/tests/test_Mkldnn.cpp

+  int ih, iw;  // oh == ow == 1
+};
+
+void testFcLayer(const testFCDesc& pm) {


testMkldnnFcLayer。这样开发者grep源码的时候，不会与普通的testFcLayer混淆了。

这里的函数已经在test_Mkldnn里面，我觉得没有再加入MKLDNN的信息了。
开发者grep也会根据文件名区分的。

luotao1 · 2017-08-09T10:28:25Z

paddle/gserver/tests/test_Mkldnn.cpp

+}
+
+TEST(MkldnnLayer, fcLayer) {
+  testFcLayer({2, 2, 3, 1, 1});


TEST(MkldnnLayer, MkldnnFcLayer) { testMkldnnFcLayer({/*bs*/ 2, /*xx*/ 2, /*xx*/ 3，...}) }

加上注释更直观。

其实在前面的有了信息定义，所以没加入，既然需求，稍后会加入。

tensor-tang

use_mkldnn_wgt这个flag目前只有在单测中使用

实际上这个在config_parser.py里面也用了，是一个从外面传递进来的信息。
现在一时也不是仅仅为单测存在的。
并且也不完全是因为pretrain，inference的时候也需要这个信息。

我认为现在可以暂时先加入这个flag，后续有了非常完整的解决weight格式问题的时候再去掉，现在也不会有需要用户去touch到这个变量。并且这个变量在紧接着的下一个PR中再来想办法，我觉得更方便。

从代码中看只是一些typedef的格式，不一定要放在父类中。

目的不是因为typedef，而是本来准备所有的layer信息都汇总在这个父类，这样也整齐好管理。
但是如果不希望看到放在父类，那么会想办法挪到别处。

这个PR的内容是有点多，因为也算是真正的第一个功能性PR，需要准备的东西很多是后面公用的，后期每个PR的内容应该会少很多。
辛苦大家review 了，谢谢。

tensor-tang · 2017-08-09T12:33:29Z

paddle/gserver/layers/MkldnnBase.h

+  DNN_SIZES,
+  DNN_FMTS,
+  DNN_ALL,
+} DNN_LOG_LEVEL;


这里会改为MKLDNN_BASE。
还是要区分下，用的场合不一样，一个用做基本信息，一个仅仅用在test里面。只是值一样罢了，如果以后觉得值想不一样，改起来也很方便。

tensor-tang · 2017-08-09T12:37:03Z

paddle/gserver/layers/MkldnnBase.h

+    }
+    // TODO(TJ): change me when mkldnn have method to reset this state
+    stream_.reset(new mkldnn::stream(mkldnn::stream::kind::eager));
+    // stream_.reset(new mkldnn::stream(mkldnn::stream::kind::lazy));


好的，我挪一下位置。thx

tensor-tang · 2017-08-09T12:41:44Z

paddle/gserver/layers/MkldnnFcLayer.cpp

+  int batchSize = input.getBatchSize();
+  if (bs_ == batchSize) {
+    return;
+  }


后面的量一旦init之后，是不会变的，在FC里面会影响weight大小。
这一点我已经在code这里有过注释了

tensor-tang · 2017-08-09T12:53:14Z

paddle/gserver/layers/MkldnnLayer.cpp

+
+#include "MkldnnLayer.h"
+
+using mem = mkldnn::memory;  // NOLINT


ok，会调整。但是我觉得如果要改，mklmem也不好吧。采用use namespace吧，会把先都改为memory。

tensor-tang · 2017-08-09T12:55:03Z

paddle/gserver/layers/MkldnnLayer.cpp

+  outGrad_->set_data_handle(topDiff);
+
+  stream_->submit(pipelineBwd_);
+}


那么我来调整下，想办法放到fc中。
@luotao1 @hedaoyuan

tensor-tang · 2017-08-09T13:04:33Z

paddle/gserver/tests/test_Mkldnn.cpp

+  int ih, iw;  // oh == ow == 1
+};
+
+void testFcLayer(const testFCDesc& pm) {


这里的函数已经在test_Mkldnn里面，我觉得没有再加入MKLDNN的信息了。
开发者grep也会根据文件名区分的。

tensor-tang · 2017-08-09T13:08:24Z

paddle/gserver/tests/test_Mkldnn.cpp

+      {INPUT_DATA,
+       "layer_0",
+       /* size of input layer= */ size_t(pm.ic * pm.ih * pm.iw),
+       /* size of weight= */ size_t(pm.oc * pm.ic * pm.ih * pm.iw)});


这里weigth的size没有问题。请参考FC的实现原理。

tensor-tang · 2017-08-09T13:09:33Z

paddle/gserver/tests/test_Mkldnn.cpp

+}
+
+TEST(MkldnnLayer, fcLayer) {
+  testFcLayer({2, 2, 3, 1, 1});


其实在前面的有了信息定义，所以没加入，既然需求，稍后会加入。

tensor-tang · 2017-08-09T13:14:58Z

python/paddle/trainer/config_parser.py

+            if use_mkldnn and use_mkldnn_wgt:
+                dims = [self.config.size, input_layer.size]
+            else:
+                dims = [input_layer.size, self.config.size]


代码风格问题，如果觉得那种方式好，会按照你的需求改动。

tensor-tang · 2017-08-09T13:21:02Z

paddle/gserver/layers/MkldnnBase.h

这里参考是这里
后续可能还会考虑FPGA的engine。

luotao1

use_mkldnn_wgt这个flag如果改起来比较麻烦，建议在下一个PR先解决这个问题，再加入新layer。
batch_norm还是建议删掉，如果要考虑后面的问题，那每个PR要考虑的会太多。我们无法预测后面出现的问题，所以每个PR应遵循“less is more”的设计思想。
关于mkldnnfclayer.cpp的写法 @hedaoyuan @Xreki 有什么意见么

luotao1 · 2017-08-10T02:30:29Z

paddle/gserver/tests/MkldnnTester.h

+  enum {
+    DNN = 0,
+    REF = 1,
+    NUM = 2,


enum每行需要注释，特别是NUM表示的是什么，这个词太普通了，所以需要注释。

luotao1 · 2017-08-10T02:33:21Z

paddle/gserver/tests/MkldnnTester.h

+   * Get delta percent
+   * if many(>failRate) wrong(abs(dnn-ref)/abs(ref)>thres) points return the
+   * max(diff/ref)
+   * else return sum(abs(a-b)) / sum(abs(b)) should smaller than eps


* if many(>failRate) wrong(abs(dnn-ref)/abs(ref)>thres) points: * return the max(diff/ref) * else: * return sum(abs(a-b)) / sum(abs(b))

should smaller than eps这句表示什么？和前面的return连起来不通

luotao1 · 2017-08-10T03:05:37Z

paddle/gserver/tests/CMakeLists.txt

+    add_unittest_without_exec(test_Mkldnn
+        test_Mkldnn.cpp
+        MkldnnTester.cpp
+        LayerGradUtil.cpp)


test_mkldnn依赖layerGradUtil.cpp么，从代码上看，是完全独立的，26行可以去掉了吧。

luotao1 · 2017-08-10T03:22:23Z

paddle/gserver/layers/MkldnnFcLayer.h

+  size_t iLayerSize_;  // == ic * ih * iw
+
+  bool hasInitedWgt_;
+  bool hasSpatial_;


hasSpatial_这个变量用来做什么？求注释。

tensor-tang

Rename done.
Mkldnn => MKLDNN and Cpu => CPU

tensor-tang · 2017-08-10T05:10:54Z

paddle/gserver/layers/MkldnnBase.h

tensor-tang · 2017-08-10T05:11:13Z

paddle/gserver/layers/MkldnnFcLayer.cpp

tensor-tang · 2017-08-10T05:11:29Z

paddle/gserver/layers/MkldnnFcLayer.h

tensor-tang · 2017-08-10T05:11:42Z

paddle/gserver/layers/MkldnnLayer.h

tensor-tang · 2017-08-10T05:11:59Z

paddle/gserver/layers/MkldnnLayer.h

tensor-tang · 2017-08-10T05:12:20Z

paddle/gserver/tests/MkldnnTester.cpp

tensor-tang · 2017-08-10T05:50:34Z

@luotao1

enum每行需要注释，特别是NUM表示的是什么，这个词太普通了，所以需要注释。
Thanks， done。

should smaller than eps这句表示什么？和前面的return连起来不通
这里的意思是返回值需要小于一个eps，才表示他测试的case过了。会再加一行说明。

test_mkldnn依赖layerGradUtil.cpp么，从代码上看，是完全独立的，26行可以去掉了吧。
实际还是依赖的，在MKLDNNTester.h里面包含了LayerGradUtil.h
因为在reset()里面初始化data layer和test layer用到了基本的函数initDataLayer和initTestLayer

hasSpatial_这个变量用来做什么？

这个变量是用来判断输入的layer包不包含图像信息，就是iw和ih是不是有值，且都不为1。
这个class里面多处需要用到所以用做成员变量。会加上注释。thx

use_mkldnn_wgt这个flag如果改起来比较麻烦，建议在下一个PR先解决这个问题，再加入新layer。

没问题，我觉得可以提一个task在project里面或者提一个issue，我下一个PR要先解决这件事情。因为这个flag对我们都还挺重要的。

batch_norm还是建议删掉，如果要考虑后面的问题，那每个PR要考虑的会太多。我们无法预测后面出现的问题，所以每个PR应遵循“less is more”的设计思想。

ok，我先注释掉，然后加一个TODO在这里

Done.

luotao1

LGTM

luotao1 · 2017-08-10T12:49:47Z

paddle/gserver/layers/MKLDNNFcLayer.h

  bool hasInitedWgt_;
+
+  // if input layer has image size info (ih>1 && iw>1)
  bool hasSpatial_;


照注释的意思，这个变量改成：hasImgSizeInfo_类似的更加直观。或者其他名字也可。

luotao1 · 2017-08-10T12:50:18Z

paddle/gserver/tests/MKLDNNTester.h

-    NUM = 2,
+    DNN = 0,  // MKLDNN layer
+    REF = 1,  // Reference layer
+    NUM = 2,  // Number of total


total是啥？Number of MKLDNN layer and Reference layer?

luotao1 · 2017-08-10T12:51:01Z

paddle/gserver/tests/MKLDNNTester.h

   * max(diff/ref)
-   * else return sum(abs(a-b)) / sum(abs(b)) should smaller than eps
+   * else return sum(abs(a-b)) / sum(abs(b))
+   * The return value should smaller than eps when passing.


The return value should be （缺少动词）

tensor-tang · 2017-08-10T13:06:57Z

hasSpatial_ 这个变量，我感觉还是维持现状比较好，因为其实我参考的这里

total是啥？Number of MKLDNN layer and Reference layer?

嗯表示的是一共就这两种，主要是为了避免magic number的存在，所以起了一个简单短小的名字。

The return value should be （缺少动词）

嗯嗯，thx。如果问题不大的话，我下次一起改掉。

Thanks

tensor-tang added 10 commits August 4, 2017 13:33

add mkldnn fc files

def959a

add use_mkldnn flag

3c3a11a

fix mkldnn lib bug, and mkldnnbase

94b172a

add mkldnn fc forward

90d5be7

add mkldnn fc backward

1203ebc

add mkldnn tester

ec9009f

pass mkldnn gtest

0c95117

add test case use_mkldnn_wgt

6373291

skip reset mkldnn when input size does not change

e18fbd8

Merge remote-tracking branch 'upstream/develop' into merge

abc49f7

tensor-tang requested a review from luotao1 August 8, 2017 13:56

wangkuiyi requested changes Aug 8, 2017

View reviewed changes

remove unused comments, refine and rename

f6a9409

tensor-tang force-pushed the merge branch from d678dae to f6a9409 Compare August 9, 2017 06:11

luotao1 reviewed Aug 9, 2017

View reviewed changes

luotao1 requested review from Xreki and hedaoyuan August 9, 2017 07:41

tensor-tang commented Aug 9, 2017

View reviewed changes

luotao1 reviewed Aug 9, 2017

View reviewed changes

tensor-tang commented Aug 9, 2017

View reviewed changes

rename and refine functions

b2bd671

luotao1 reviewed Aug 10, 2017

View reviewed changes

tensor-tang commented Aug 10, 2017

View reviewed changes

tensor-tang added 2 commits August 10, 2017 13:37

rename files and classes, use uppercase of Mkldnn and Cpu

a475a57

add comments and todo lists

2d4c66d

tensor-tang force-pushed the merge branch from 37d36cb to 2d4c66d Compare August 10, 2017 05:50

luotao1 approved these changes Aug 10, 2017

View reviewed changes

wangkuiyi approved these changes Aug 11, 2017

View reviewed changes

Xreki merged commit 2e87d74 into PaddlePaddle:develop Aug 11, 2017

tensor-tang deleted the merge branch August 11, 2017 03:20

luotao1 mentioned this pull request Aug 11, 2017

remove use_mkldnn_wgt flag #3409

Closed

luotao1 mentioned this pull request Jun 4, 2018

Mkldnn layout #11040

Merged

tensor-tang mentioned this pull request Jun 4, 2018

rename Mkldnn to MKLDNN #11147

Merged


		#include "MkldnnLayer.h"

		using mem = mkldnn::memory; // NOLINT

Enable mkldnn_fc for general format #3337

Enable mkldnn_fc for general format #3337

Uh oh!

Conversation

tensor-tang commented Aug 8, 2017 • edited by luotao1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wangkuiyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tensor-tang Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tensor-tang Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tensor-tang commented Aug 9, 2017

Uh oh!

luotao1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tensor-tang commented Aug 8, 2017 •

edited by luotao1

Loading

tensor-tang Aug 10, 2017 •

edited

Loading

tensor-tang Aug 9, 2017 •

edited

Loading