-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[wasm] Add Conv3D kernels #7466
Conversation
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: 0 of 1 approvals obtained (waiting on @chunnienc, @Linchenn, and @mattsoulanille)
tfjs-backend-wasm/src/cc/kernels/AvgPool3DGrad.cc
line 68 at r1 (raw file):
.effective_filter_height = effective_filter_height, .effective_filter_width = effective_filter_width, .pad_front = effective_filter_depth - 1 - pad_front,
are these change related?
tfjs-backend-wasm/src/cc/kernels/MaxPool3DGrad.cc
line 88 at r1 (raw file):
[](const std::pair<float, int>& data) { return data.second; }); pool3d_info.pad_front = effective_filter_depth - 1 - pad_front;
same here
This PR adds a new Conv3d backprop impl with forward conv info, which computes the new pads for backprop in the backprop function. The pool3d part is changed to align to this new style of defining backprop function with conv/pool info from JS. |
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 13 of 13 files at r1, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @chunnienc, @Linchenn, and @mattsoulanille)
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.
LGTM
Reviewed 3 of 13 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @chunnienc, @Linchenn, and @pyu10055)
tfjs-backend-wasm/src/cc/conv3d_impl.h
line 100 at r2 (raw file):
for (int yc = 0; yc < info.out_width; ++yc) { int xc_corner = yc * info.stride_width - info.pad_left; for (int wc = 0; wc < info.filter_width; ++wc) {
Nit: Would you get better CPU cache hits if you put the filter width and height loops right next to each other, since filter is contiguous in memory? Right now, they're separated by the output width loop. I'm not an expert at this, so I could be totally wrong.
Actually, this might be better to do in a different PR, since this one appears to be directly implementing the CPU backend's algorithm in WASM.
Code quote:
for (int wr = 0; wr < info.filter_height; ++wr) {
int xr = xr_corner + wr * info.dilation_height;
if (xr < 0 || xr >= info.in_height) {
continue;
}
for (int yc = 0; yc < info.out_width; ++yc) {
int xc_corner = yc * info.stride_width - info.pad_left;
for (int wc = 0; wc < info.filter_width; ++wc) {
tfjs-backend-wasm/src/cc/kernels/AvgPool3DGrad.cc
line 68 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
are these change related?
This is factoring out the pad calculations to pool3d_impl.h
@pyu10055 I'm not sure why Reviewable requested your review again when I submitted mine. I don't see any relevant changes since your last review. |
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is