[oneDNN][INT8][FP32] INT8/FP32 elementwise_mul and INT8 elementwise_add #24812

Sand3r- · 2020-05-29T13:22:22Z

PR types: New features

PR changes: OPs

Describe:

PR introduces oneDNN forward kernels for:

INT8 elementwise_add
INT8 elementwise_mul
FP32 elementwise_mul

It also fixes 2 bugs:

When using ShareBufferWith (inplace operators), the type wasn't assigned to the tensor to which the buffer was shared with. This resulted in hard to detect memory crashes when attempting to test inplace elementwise int8 operators (size was allocated in ints, while fetch_op required size allocated for floats).
OpTester has can now succesfully output uint8 results to the console. Previously arrays of uint8 data were treated as encoded byte strings instead of arrays of numbers.

At the same time, the following has been removed:

An old elementwise_mul kernel that was crafted for a special case in ResNeXt-50. This special case (and many others) should now be fully covered inside of the new kernel
It's unit tests

PRs to be merged before this one:

#24853
#24854
#24855

Allow mkldnn_reuse to create primitive descriptors with attributes

test=develop

paddle-bot-old · 2020-05-29T13:22:30Z

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

Sand3r- · 2020-06-01T08:24:01Z

MAC_PR_CI_python35 (Paddle Mac) -> Fails due to missing approval for removal of UT. UT test_elementwise_mul_op_correct_dims was removed because its no longer relevant (old elementwise_mul kernel was removed).
PR_CI_CPU_Py2 (Compile) -> Fails due to missing approvals for:

Change in API - I don't know which one is meant here, maybe addition of int8 attributes to elementwise_op?
Change in tensor.h (fixed bug with ShareBufferWith)
Setting dygraph field to false in int8 UTs, just the way it is in original elementwise_mul test, otherwise the oneDNN tests fail with dygraph
Usage of skip_check_grad_ci for int8 inference-only ops.

PR_Windows_CI (Paddle Windows)

[14:33:59]W:	 [Step 7/8] CMake Error at cmake/inference_lib.cmake:217 (add_dependencies):
[14:33:59]W:	 [Step 7/8]   The dependency target "cuda_error_proto" of target "fluid_lib_dist" does
[14:33:59]W:	 [Step 7/8]   not exist.
[14:33:59]W:	 [Step 7/8] Call Stack (most recent call first):
[14:33:59]W:	 [Step 7/8]   CMakeLists.txt:211 (include)

No idea what happened, didn't touch anything there.

PR-CI-Coverage:

[2020-04-03 21:24:05]  [  3%] Building CXX object paddle/fluid/framework/io/CMakeFiles/shell.dir/shell.cc.o
[2020-04-03 21:24:05]  [  3%] Linking CXX static library libfs.a
[2020-04-03 21:24:05]  In file included from /paddle/paddle/fluid/platform/enforce.h:41:0,
[2020-04-03 21:24:05]                   from /paddle/paddle/fluid/framework/io/shell.cc:16:
[2020-04-03 21:24:05]  /paddle/paddle/fluid/platform/errors.h:23:50: fatal error: paddle/fluid/platform/error_codes.pb.h: No such file or directory
[2020-04-03 21:24:05]  #include "paddle/fluid/platform/error_codes.pb.h"
[2020-04-03 21:24:05]                                                    ^
[2020-04-03 21:24:05]  compilation terminated.

Again, no idea what went wrong there, didn't touch the file.

@luotao1 do you know if there are any ongoing fixes for PR-CI-Coverage and PR_Windows_CI (Paddle Windows)?

@jczaja @wojtuss you can start the review, as no tests currently fail, CIs only fail because of missing approvals, or random PR-unrelated compilation failures.

luotao1 · 2020-06-01T09:06:57Z

PR-CI-Coverage and PR_Windows_CI fails

fixed by #24800, just rerun it, and I have already rerun Windows.

Change in API - I don't know which one is meant here, maybe the addition of int8 attributes to elementwise_op?

It seems not the addition of int8 attributes, I have rerun it at first.

jczaja

LGTM

test=develop

wojtuss

LGTM

luotao1 · 2020-06-02T03:12:29Z

Could you split this PR into several ones? The reason is:

There are too many approvals in this PR.
Change in API - I don't know which one is meant here, maybe addition of int8 attributes to elementwise_op?
- After rerun, it still has the Change in API. If you can split the PR into smaller ones, it may be easier for us to find the reason.

Sand3r- · 2020-06-02T07:36:43Z

@luotao1

Could you split this PR into several ones? The reason is:

There are too many approvals in this PR.

Change in API - I don't know which one is meant here, maybe addition of int8 attributes to elementwise_op?

After rerun, it still has the Change in API. If you can split the PR into smaller ones, it may be easier for us to find the reason.

Following files are strictly connected to one another and concern addition of elementwise_kernels:

 paddle/fluid/operators/elementwise/CMakeLists.txt 
 paddle/fluid/operators/elementwise/elementwise_mul_op.h 
 paddle/fluid/operators/elementwise/elementwise_op.h 
 paddle/fluid/operators/elementwise/mkldnn/elementwise_add_mkldnn_op.cc 
 paddle/fluid/operators/elementwise/mkldnn/elementwise_mkldnn_op.h
 paddle/fluid/operators/elementwise/mkldnn/elementwise_mul_mkldnn_op.cc
 paddle/fluid/operators/elementwise/test_elementwise_mul_op_dim.cc
 paddle/fluid/platform/mkldnn_reuse.h
 python/paddle/fluid/tests/unittests/mkldnn/test_elementwise_add_mkldnn_op.py 
 python/paddle/fluid/tests/unittests/mkldnn/test_elementwise_mul_mkldnn_op.py

This is a bugfix required for int8 tests to pass

 python/paddle/fluid/tests/unittests/op_test.py

This is a bugfix required for int8 inplace tests to pass

 paddle/fluid/framework/tensor.h

Do you want me to split this into 3 such PRs?

luotao1 · 2020-06-02T08:10:12Z

It's better to split this into 3 such PRs. Besides, how about remove test_elementwise_mul_op_dim.cc in a separate PR, which is easy for QA review?

Sand3r- · 2020-06-02T08:23:37Z

@luotao1

It's better to split this into 3 such PRs. Besides, how about remove test_elementwise_mul_op_dim.cc in a separate PR, which is easy for QA review?
Alright, will do.

Sand3r- · 2020-06-02T08:44:25Z

@luotao1 Done:
#24853
#24854
And the current one.

Btw. removing test_elementwise_mul_op_dim.cc separately might cause PR_CI_Coverage to fail. Are you sure we should move it to separate PR? Also: Maintaining so many PRs is going to be quite of a hassle.

luotao1 · 2020-06-02T08:50:33Z

removing test_elementwise_mul_op_dim.cc separately might cause PR_CI_Coverage to fail.

The fail reason for PR_CI_Coverage is an approval. Is any other fail reason?

Are you sure we should move it to separate PR?

Yes, it's better to move it to separate PR, which is easy for QA review. And your current PR is much more clearer.

Maintaining so many PRs is going to be quite of a hassle.

I see the other two PRs, which are quick to be merged. But if they are in one PR, the approval may take some time.

Sand3r- · 2020-06-02T09:05:57Z

Yes, it's better to move it to separate PR, which is easy for QA review. And your current PR is much more clearer.

Alright, I moved it to #24855

I see the other two PRs, which are quick to be merged. But if they are in one PR, the approval may take some time.

That's assuming no random CI failures 😉

jczaja · 2020-06-09T12:54:05Z

Work on this functionality is continued in PR #24984. Closing

Sand3r- added 10 commits May 29, 2020 14:29

Start implementing int8 eltwise add

f9030ba

Add int8 attributes to elementwise_op

cca7255

Fix scale computation, extend dnnl reuse functionality

72487ff

Allow mkldnn_reuse to create primitive descriptors with attributes

Add INT8 UnitTests

2914ad2

Fix OpTester from incorrectly converting uint8 data to utf-8 string

bd008e8

Fix bug in ShareBufferWith causing eltwise_add inplace ut to fail

b2a513d

test=develop

Fix failing broadcast UTs

d36c699

test=develop

Extract elementwise kernel to a header

c5b5efc

Implement INT8/FP32 oneDNN elementwise_mul kernel

e369631

test=develop

Fix header path

f9919af

test=develop

Sand3r- requested review from jczaja and wojtuss May 29, 2020 13:22

wojtuss added the Intel label Jun 1, 2020

Sand3r- added the int8 label Jun 1, 2020

jczaja reviewed Jun 1, 2020

View reviewed changes

Force mkldnn elementwise tests to not run on cuda

fedbaee

test=develop

wojtuss approved these changes Jun 1, 2020

View reviewed changes

Sand3r- mentioned this pull request Jun 2, 2020

Remove old mkldnn_elementwise_mul test #24855

Merged

This was referenced Jun 4, 2020

[MKL-DNN] Reimplement elementwise_mul using Binary primitive of MKL-DNN #21029

Closed

[oneDNN]elementwise_add and elementwise_mul int8 support #24984

Merged

jczaja closed this Jun 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[oneDNN][INT8][FP32] INT8/FP32 elementwise_mul and INT8 elementwise_add #24812

[oneDNN][INT8][FP32] INT8/FP32 elementwise_mul and INT8 elementwise_add #24812

Sand3r- commented May 29, 2020 •

edited

Loading

paddle-bot-old bot commented May 29, 2020

Sand3r- commented Jun 1, 2020

luotao1 commented Jun 1, 2020

jczaja left a comment

wojtuss left a comment

luotao1 commented Jun 2, 2020

Sand3r- commented Jun 2, 2020

luotao1 commented Jun 2, 2020

Sand3r- commented Jun 2, 2020

Sand3r- commented Jun 2, 2020

luotao1 commented Jun 2, 2020

Sand3r- commented Jun 2, 2020

jczaja commented Jun 9, 2020

[oneDNN][INT8][FP32] INT8/FP32 elementwise_mul and INT8 elementwise_add #24812

[oneDNN][INT8][FP32] INT8/FP32 elementwise_mul and INT8 elementwise_add #24812

Conversation

Sand3r- commented May 29, 2020 • edited Loading

paddle-bot-old bot commented May 29, 2020

Sand3r- commented Jun 1, 2020

luotao1 commented Jun 1, 2020

jczaja left a comment

Choose a reason for hiding this comment

wojtuss left a comment

Choose a reason for hiding this comment

luotao1 commented Jun 2, 2020

Sand3r- commented Jun 2, 2020

luotao1 commented Jun 2, 2020

Sand3r- commented Jun 2, 2020

Sand3r- commented Jun 2, 2020

luotao1 commented Jun 2, 2020

Sand3r- commented Jun 2, 2020

jczaja commented Jun 9, 2020

Sand3r- commented May 29, 2020 •

edited

Loading