-
Notifications
You must be signed in to change notification settings - Fork 2k
[wasm] Add relu, relu6 kernels. #2432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dsmilkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 8 files at r1.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @annxingyuan and @dsmilkov)
tfjs-backend-wasm/src/setup_test.ts, line 52 at r1 (raw file):
'gradient', // Not yet implemented. 'valueAndGradients', // Not yet implemented. 'fused', // Not yet implemented.
curious what tests are behind this fused bit?
tfjs-backend-wasm/src/cc/kernels/Relu.cc, line 25 at r1 (raw file):
namespace { inline float oper(const float val) { return val < 0. ? 0. : val; }
can you add a TODO, here and in Relu6 below, to replace these relu and relu6 with xnnpack_clamp. The reason why that is better in the long run is because xnnpack will take care of calling SIMD instructions/acceleration and multi-threading
tfjs-backend-wasm/src/cc/kernels/Relu6.cc, line 26 at r1 (raw file):
namespace { float max = 6.;
since this is a hard-coded constant, rename to kMax (the k prefix tells the reader it's a constant). See https://google.github.io/styleguide/cppguide.html#Constant_Names
dsmilkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @annxingyuan and @dsmilkov)
tfjs-backend-wasm/src/cc/kernels/Relu.cc, line 25 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
can you add a TODO, here and in Relu6 below, to replace these relu and relu6 with xnnpack_clamp. The reason why that is better in the long run is because xnnpack will take care of calling SIMD instructions/acceleration and multi-threading
cc @Maratyszcza Curious does xnnpack clamp do something special to accelerate? does it use wasm simd instrincis if enabled?
dsmilkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @annxingyuan)
tfjs-backend-wasm/src/cc/kernels/Relu6.cc, line 26 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
since this is a hard-coded constant, rename to
kMax(the k prefix tells the reader it's a constant). See https://google.github.io/styleguide/cppguide.html#Constant_Names
also it's important to mark it as constant, otherwise the compiler can't inline it
annxingyuan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @dsmilkov)
tfjs-backend-wasm/src/setup_test.ts, line 52 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
curious what tests are behind this fused bit?
Changed to be more specific about what to exclude.
tfjs-backend-wasm/src/cc/kernels/Relu.cc, line 25 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
cc @Maratyszcza Curious does xnnpack clamp do something special to accelerate? does it use wasm simd instrincis if enabled?
Done
tfjs-backend-wasm/src/cc/kernels/Relu6.cc, line 26 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
also it's important to mark it as constant, otherwise the compiler can't inline it
Done
dsmilkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2.
Reviewable status:complete! 1 of 1 approvals obtained
Changes
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is