add NNPACK check status and test #198

edgarriba · 2016-06-21T09:36:37Z

@nyanp @wangyida @bhack @mtamburrano
Could you test NNPACK in your own devices? Getting a segfault here

bhack · 2016-06-21T15:59:57Z

The Segfault was on the AVX2 flag?

edgarriba · 2016-06-21T16:23:12Z

nope. Still not working

edgarriba · 2016-06-21T17:14:10Z

tiny_cnn/core/backend_nnp.h

        deconv_layer_worker_specific_storage& cws =
            (*deconv_layer_worker_storage_)[index];
-        copy_and_pad_delta(cws.curr_delta_padded, *in_grad[0]);
+        copy_and_pad_delta(cws.curr_delta_padded, *in_grad[0]); //  *in_grad[0] does not exist here!
        const vec_t& W    = *in_data[1];


@wangyida Are you sure that you compiled this? There are variables that are not declared

Here is a mistake, line 101:

copy_and_pad_delta(cws.curr_delta_padded, *in_grad[0]);

should be replaced as:

(*deconv_layer_worker_storage_)[index].prev_out_ = in_data[0];

and line 166:

std::function<void(const vec_t&, vec_t&)> copy_and_pad_delta;

should be replaced as:

std::function<void(const vec_t&, int)> copy_and_unpad_output;

You can modify them directly, output of deconvolution might be unpadded and the delta in backforward might be padded, it's just the opposite operation of convolution in padding strategy.
I haven't compile with nnp backend as nnPack is not installed yet. These codes are modified logically in nnp.

bhack · 2016-06-21T18:45:19Z

@edgarriba How the segfault is reproducible?

bhack · 2016-06-21T20:13:11Z

@edgarriba I've executed the test and i got an error. Have you executed init_nnp_engine()?

bhack · 2016-06-21T20:13:53Z

terminate called after throwing an instance of 'tiny_cnn::nn_error'
  what():  ����m[ERROR] NNPACK Max-Pool requires a stride == 2.

edgarriba · 2016-06-21T21:02:23Z

good point ! Unfortunately my laptop doesn't support AVX2.

If I compile with -mavx2 flag I got the following error: Illegal instruction (core dumped). In the other case, NNPACK raises the following error: NNPACK does not implement this function for the host CPU.

bhack · 2016-06-21T21:14:17Z

Arg! We have avx2 at work. But I've only avx on laptop.
Avx2 require a quite new CPU. How many kernels are avx2 only?

bhack · 2016-06-21T21:15:48Z

P.s. Avx2 was introduced with Haswell arch.

bhack · 2016-06-22T08:20:46Z

@Maratyszcza Needs all the kernels in NNPACK avx2?

bhack · 2016-06-22T09:05:25Z

@edgarriba You can execute tests with Intel SDE or Qemu to emulate avx2

edgarriba · 2016-06-22T09:08:11Z

@bhack oki. I'll try

edgarriba · 2016-06-22T11:11:05Z

@bhack getting the same. I updated the test, the parameters were wrong.
Not sure if we are configuring well the kernels. The segfault occurs inside NNPACK routine.

@Maratyszcza @ajtulloch Did you found any issue during Caffe's integration?

ajtulloch · 2016-06-22T13:10:58Z

Make sure all the unit tests for the Caffe integration pass on your machine?

edgarriba · 2016-06-22T17:02:12Z

@ajtulloch thx for your answer.
I've tried nnpack-pr branch and all tests passes except InnerProductLayerTest/0.TestGradientNNPACK. Check log.txt

@bhack @nyanp If tests for convolution and pooling works in Caffe's version it means that we are doing something wrong.

bhack · 2016-06-22T17:04:48Z

@edgarriba Are you running with Intel SDE?

edgarriba · 2016-06-22T19:23:55Z

@ajtulloch @Maratyszcza I noticed that nnp_max_pooling_output is running, however it deallocates the content of my output_ptr. Any idea?

Maratyszcza · 2016-06-22T19:28:57Z

@edgarriba An issue in convolution backpropagation came up during Caffe integration (see Maratyszcza/NNPACK#15), but I believe it is fixed now. AFAIK, there aren't any unsolved crash or correctness bugs.

As for nnp_max_pooling_output, you may want to check how you compute the output size (and compare it with NNPACK). I remember that there are some variations in output size between packages, and NNPACK follows Caffe & cuDNN convention. Update: if your package follows different convention, it may lead to out-of-bounds writes in NNPACK, which may make it feel like some memory is deallocated.

Maratyszcza · 2016-06-22T19:30:41Z

@bhack I'd expect that all kernels require AVX2 or at least FMA3 (which means Haswell+ among Intel CPUs). However, you may build NNPACK with clang with --enable-psimd flag. Performance wouldn't be as good, but it will work everywhere, and probably suit your development needs.

bhack · 2016-06-22T20:10:22Z

@Maratyszcza Thank you. I've already executed with Intel sde on a cpu without avx2 but your option it is useful cause doen't rely on closed source tools. There will be a common maintainer ship of NNPACK with the Facebook group or will remain a personal project?

Maratyszcza · 2016-06-22T21:56:54Z

@bhack I heard you can emulate new instruction sets with qemu if Intel SDE doesn't suit your needs.

For the next year I am still a Ph.D. student at Georgia Tech, but NNPACK is developed in collaboration with F.A.I.R., and it is production-quality code.

wangyida · 2016-06-24T02:14:42Z

This is a major PR, so I think that some bug fixes in my local machine is better not being pushed as a new PR that will make it a additional task for @edgarriba to rebase again. I think I could push them after this PR got merged, am I right?
BTW, Line 141 of CMakeLists.txt might be better replaced as:

LINK_LIBRARIES(${REQUIRED_LIBRARIES})

Some OpenCV related libs would be regarded placed in the current directory if it is written as:

link_directories(${REQUIRED_LIBRARIES})

add AVX2 flag add NNPACK support for maxpooling layer add missing engine initialization minor typo and lint changes fix minor issue with param access refactor backend_init, add setup tests for maxpol and conv layers remove old file remove old file add NNPACK fully_connected fix NNPACK pointer cast fix travis checks

edgarriba · 2016-06-25T21:42:53Z

@bhack @nyanp NNPACK working. We're good to merge. Next LibDNN.

@Maratyszcza @ajtulloch thx for your support !
@Maratyszcza Can we link tiny-cnn to NNPACK project readme?

nyanp · 2016-06-26T13:09:23Z

@edgarriba
OK, great!! (I'm sorry but I wasn't enough time for the past few days to support your debbuging...) Let's go next :)

Maratyszcza · 2016-07-01T08:16:35Z

@edgarriba Referenced tiny-cnn in NNPACK readme. Or did you mean something else?

edgarriba · 2016-07-01T08:44:39Z

@Maratyszcza That's fine. Thanks a lot !

add AVX2 flag add NNPACK support for maxpooling layer add missing engine initialization minor typo and lint changes fix minor issue with param access refactor backend_init, add setup tests for maxpol and conv layers remove old file remove old file add NNPACK fully_connected fix NNPACK pointer cast fix travis checks

edgarriba force-pushed the feat/generic-computational-graph-device-abstraction branch 2 times, most recently from 0c676ea to be7711a Compare June 21, 2016 15:30

edgarriba reviewed Jun 21, 2016
View reviewed changes

nyanp mentioned this pull request Jun 22, 2016

Branching policy in tiny-cnn #186

Open

edgarriba force-pushed the feat/generic-computational-graph-device-abstraction branch 3 times, most recently from 0810832 to 0588bc8 Compare June 25, 2016 21:39

nyanp merged commit 5a14ee1 into tiny-dnn:feat/generic-computational-graph Jun 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add NNPACK check status and test #198

add NNPACK check status and test #198

edgarriba commented Jun 21, 2016 •

edited

Loading

bhack commented Jun 21, 2016

edgarriba commented Jun 21, 2016

edgarriba Jun 21, 2016

wangyida Jun 22, 2016 •

edited

Loading

bhack commented Jun 21, 2016

bhack commented Jun 21, 2016

bhack commented Jun 21, 2016

edgarriba commented Jun 21, 2016

bhack commented Jun 21, 2016

bhack commented Jun 21, 2016

bhack commented Jun 22, 2016

bhack commented Jun 22, 2016 •

edited

Loading

edgarriba commented Jun 22, 2016 •

edited

Loading

edgarriba commented Jun 22, 2016 •

edited

Loading

ajtulloch commented Jun 22, 2016

edgarriba commented Jun 22, 2016

bhack commented Jun 22, 2016

edgarriba commented Jun 22, 2016

Maratyszcza commented Jun 22, 2016 •

edited

Loading

Maratyszcza commented Jun 22, 2016

bhack commented Jun 22, 2016

Maratyszcza commented Jun 22, 2016

wangyida commented Jun 24, 2016

edgarriba commented Jun 25, 2016

nyanp commented Jun 26, 2016

Maratyszcza commented Jul 1, 2016

edgarriba commented Jul 1, 2016

add NNPACK check status and test #198

add NNPACK check status and test #198

Conversation

edgarriba commented Jun 21, 2016 • edited Loading

bhack commented Jun 21, 2016

edgarriba commented Jun 21, 2016

edgarriba Jun 21, 2016

Choose a reason for hiding this comment

wangyida Jun 22, 2016 • edited Loading

Choose a reason for hiding this comment

bhack commented Jun 21, 2016

bhack commented Jun 21, 2016

bhack commented Jun 21, 2016

edgarriba commented Jun 21, 2016

bhack commented Jun 21, 2016

bhack commented Jun 21, 2016

bhack commented Jun 22, 2016

bhack commented Jun 22, 2016 • edited Loading

edgarriba commented Jun 22, 2016 • edited Loading

edgarriba commented Jun 22, 2016 • edited Loading

ajtulloch commented Jun 22, 2016

edgarriba commented Jun 22, 2016

bhack commented Jun 22, 2016

edgarriba commented Jun 22, 2016

Maratyszcza commented Jun 22, 2016 • edited Loading

Maratyszcza commented Jun 22, 2016

bhack commented Jun 22, 2016

Maratyszcza commented Jun 22, 2016

wangyida commented Jun 24, 2016

edgarriba commented Jun 25, 2016

nyanp commented Jun 26, 2016

Maratyszcza commented Jul 1, 2016

edgarriba commented Jul 1, 2016

edgarriba commented Jun 21, 2016 •

edited

Loading

wangyida Jun 22, 2016 •

edited

Loading

bhack commented Jun 22, 2016 •

edited

Loading

edgarriba commented Jun 22, 2016 •

edited

Loading

edgarriba commented Jun 22, 2016 •

edited

Loading

Maratyszcza commented Jun 22, 2016 •

edited

Loading