Skip to content

[MKL-DNN] Fully Connected#15226

Merged
luotao1 merged 35 commits intoPaddlePaddle:developfrom
Sand3r-:mgallus/fc-mkldnn
May 24, 2019
Merged

[MKL-DNN] Fully Connected#15226
luotao1 merged 35 commits intoPaddlePaddle:developfrom
Sand3r-:mgallus/fc-mkldnn

Conversation

@Sand3r-
Copy link
Contributor

@Sand3r- Sand3r- commented Jan 8, 2019

Introduces Fully Connected operator. It has a comparable performance to the reference version, although it is necessary to be merged, so that the base for the int8 implementation and its performance gains is established.

Update 05.04.2019
Compared to a reference FC, the MKL-DNN version provides:

  • 51% speedup on SKX 6148
  • 35% on CLX 8280

@tensor-tang
Copy link
Contributor

Could you tell us the difference or improvment over the current fc implementation.

Does it just for infer?

If it is performance improvement, could you give some brief data of before and after?

@Sand3r- Sand3r- changed the title [MKL-DNN] Fully Connected [WIP][MKL-DNN] Fully Connected Jan 11, 2019
@Sand3r- Sand3r- force-pushed the mgallus/fc-mkldnn branch 3 times, most recently from dbe2aeb to e7fb740 Compare January 15, 2019 13:41
@Sand3r- Sand3r- force-pushed the mgallus/fc-mkldnn branch 3 times, most recently from 01d0b0d to 471fd62 Compare January 24, 2019 10:58
@hshen14
Copy link
Contributor

hshen14 commented Feb 15, 2019

@Sand3r- any update on this?

@Sand3r-
Copy link
Contributor Author

Sand3r- commented Feb 15, 2019

@Sand3r- any update on this?

@hshen14 As mentioned in an internal mail, Paddle's CI VM yields flawed output for some of the convolutions and fc. This results in insufficient accuracy to have test_analyzer_resnet50 passed. This isn't the case for all of our local machines, where everything works as intended.

@Sand3r- Sand3r- force-pushed the mgallus/fc-mkldnn branch 4 times, most recently from 56c3ab9 to f121edc Compare February 21, 2019 10:22
@Sand3r- Sand3r- force-pushed the mgallus/fc-mkldnn branch 2 times, most recently from b14d83c to 23e7ba3 Compare February 26, 2019 11:09
@CLAassistant
Copy link

CLAassistant commented Mar 4, 2019

CLA assistant check
All committers have signed the CLA.

@Sand3r- Sand3r- force-pushed the mgallus/fc-mkldnn branch from 9be4960 to 1141d48 Compare March 4, 2019 12:00
@Sand3r- Sand3r- force-pushed the mgallus/fc-mkldnn branch 2 times, most recently from 10d06d0 to 0649019 Compare March 12, 2019 14:35
@kbinias
Copy link
Contributor

kbinias commented Mar 12, 2019

The problem with the FC operator is that on CI MKL library is using AVX2 instruction set, while it should use AVX512. We reproduced the problem locally by enforcing AVX2 in MKL using MKL_CBWR=AVX2 environment setting.
@luotao1 We need more information about CI running environment. Could you send us info about current CI configuration (results of commands: set and env) ?

@luotao1
Copy link
Contributor

luotao1 commented Mar 13, 2019

We need more information about CI running environment.

All the CI machines have AVX512, see #15032 (comment) and #15032 (comment)

Could you send us info about current CI configuration (results of commands: set and env)

Do you mean MKL_CBWR? We don't set this environment. And you can echo $MKL_CBWR yourself. Maybe you can do echo in cmake_gen

function cmake_gen() {

@Sand3r- Sand3r- force-pushed the mgallus/fc-mkldnn branch from f9cde12 to 8caea93 Compare March 13, 2019 12:34
@kbinias
Copy link
Contributor

kbinias commented Mar 13, 2019

@luotao1 I noticed differences between the downloaded versions of the MKLML libraries.
CI download: Glibc225_vsErf_mklml_lnx_2019.0.1.20181227
Public PaddlePaddle: mklml_lnx_2019.0.1.20181227
These libraries do not match at the binary level.
I am not sure if MKLML from CI support AVX512.

Sand3r- added 21 commits May 24, 2019 09:03
Also, refine pass to comply with newest interface.
test=develop
@Sand3r-
Copy link
Contributor Author

Sand3r- commented May 24, 2019

Done.

@luotao1 luotao1 merged commit 0c39b97 into PaddlePaddle:develop May 24, 2019
@luotao1 luotao1 mentioned this pull request May 24, 2019
@Sand3r-
Copy link
Contributor Author

Sand3r- commented May 24, 2019

Didn't expect the happiest day of my life would come today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants