Skip to content
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 AvgPool3D and MaxPool3D kernels #7294

Merged
merged 11 commits into from
Jan 24, 2023

Conversation

chunnienc
Copy link
Collaborator

@chunnienc chunnienc commented Jan 20, 2023

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

Copy link
Collaborator

@pyu10055 pyu10055 left a 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 and @mattsoulanille)


tfjs-backend-wasm/src/cc/pool3d_impl.h line 1 at r2 (raw file):

#ifdef __EMSCRIPTEN__

missing copyright header?


tfjs-backend-wasm/src/cc/pool3d_impl.cc line 1 at r1 (raw file):

#ifdef __EMSCRIPTEN__

missing copyright header


tfjs-backend-wasm/src/cc/pool3d_impl.cc line 45 at r1 (raw file):

};
template <typename OUT, typename IN, typename T>
inline void Pool3DImpl(

is this method used? seems to be a simplified version of NDHWCPool3DImpl?

@chunnienc
Copy link
Collaborator Author

Added license header. I should have deleted the file pool3d_impl.cc.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @chunnienc and @mattsoulanille)


tfjs-backend-wasm/src/cc/pool3d_impl.h line 32 at r3 (raw file):

    return v;
  }
  return (v % d + v) % d;

this does not guarantee non negative, right?
maybe the method name need to be updated to better reflect the logic?


tfjs-backend-wasm/src/cc/pool3d_impl.h line 78 at r3 (raw file):

};
template <typename IN, typename OUT, typename FI, typename FAP, typename FAG>
inline void NDHWCPool3DImpl(const IN* x_buf, OUT* out_buf,

like the code sharing across ops.

@chunnienc
Copy link
Collaborator Author

this does not guarantee non negative, right?
maybe the method name need to be updated to better reflect the logic?

This method is intended to get the non-negative modulo. But you're right, the implementation was wrong. Fixed, thanks for the review and pointing out.

Copy link
Member

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 9 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @chunnienc)


tfjs-backend-wasm/src/cc/pool3d_impl.h line 120 at r3 (raw file):

            int out_offset =
                info.out_offset(batch, y_depth, y_row, y_col, channel);
            out_buf[out_offset] = filter_aggregate(filter_data);

Thinking about future improvements, addition (in AvgPool3D) is associative, so I wonder if this could be parallelized or vectorized by xnnpack. That might not matter if this op is not the main source of slowness in models, though.

Code quote:

                  filter_apply(filter_data, x_buf[x_offset]);
                }
              }
            }
            int out_offset =
                info.out_offset(batch, y_depth, y_row, y_col, channel);
            out_buf[out_offset] = filter_aggregate(filter_data);

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dition (in AvgPool3D) is associative, so I wonder if this could be parallelized or vectorized by xnnpack. That might not mat
LGTM

Reviewed 8 of 9 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @chunnienc)


tfjs-backend-wasm/src/cc/pool3d_impl.h line 120 at r3 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

Thinking about future improvements, addition (in AvgPool3D) is associative, so I wonder if this could be parallelized or vectorized by xnnpack. That might not matter if this op is not the main source of slowness in models, though.

does Xnnpack have pooling op?

Copy link
Member

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @chunnienc)


tfjs-backend-wasm/src/cc/pool3d_impl.h line 120 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

does Xnnpack have pooling op?

XNNPACK has 2D Pooling ops. I don't know if they could be used as part of a 3D pooling implementation. Also, I don't know if they have a general pooling utility like the one in this PR.

https://github.com/google/XNNPACK#operator-coverage

@chunnienc
Copy link
Collaborator Author

tflite has its own implementation of pooling3d (/lite/kernels/pooling3d.cc), and there is no xnnpack delegate for it. But I think it's more because MaxPool3D and AvgPool3D are not yet built-in ops there.

Similarly, conv3d is a built-in op in tflite but xnnpack delegate does not have it either. So I assume 3d implementation with xnnpack 2d is non-trivial. I will revisit this topic (using xnnpack for 3d ops) later if there is a need.

@mattsoulanille
Copy link
Member

tflite has its own implementation of pooling3d (/lite/kernels/pooling3d.cc), and there is no xnnpack delegate for it. But I think it's more because MaxPool3D and AvgPool3D are not yet built-in ops there.

Similarly, conv3d is a built-in op in tflite but xnnpack delegate does not have it either. So I assume 3d implementation with xnnpack 2d is non-trivial. I will revisit this topic (using xnnpack for 3d ops) later if there is a need.

SGTM. Feel free to merge this whenever. I didn't mean to block it.

@chunnienc chunnienc merged commit 710b347 into tensorflow:master Jan 24, 2023
@chunnienc chunnienc deleted the wasm-avgpool3d branch January 24, 2023 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants