Skip to content

Conversation

@annxingyuan
Copy link
Contributor

@annxingyuan annxingyuan commented Nov 15, 2019

Changes

  • Implement maxPool wasm kernel that calls into XNNPACK
  • Short circuit maxpool if window is [1,1] and input/output shape match

This change is Reviewable

@annxingyuan annxingyuan requested review from Maratyszcza, dsmilkov and nsthorat and removed request for nsthorat November 18, 2019 18:10
@annxingyuan annxingyuan self-assigned this Nov 18, 2019
@annxingyuan annxingyuan changed the title WIP wasm maxPool [wasm] Add maxPool kernel. Nov 18, 2019
Copy link
Collaborator

@Maratyszcza Maratyszcza 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 @annxingyuan, @dsmilkov, and @Maratyszcza)


tfjs-backend-wasm/src/cc/kernels/MaxPool.cc, line 83 at r1 (raw file):

        flags, &max_pool_op);

    if (status != xnn_status_success) {

If xnn_create_max_pooling2d_nhwc_f32 fails, the function should exit early, because max_pool_op is nullptr, and it is not safe to call xnn_setup_max_pooling2d_nhwc_f32 and xnn_run_operator with a nullptr

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Awesome!

Reviewed 6 of 9 files at r1, 3 of 3 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan and @dsmilkov)


tfjs-backend-wasm/src/setup_test.ts, line 42 at r2 (raw file):

    include: 'maxPool',
    excludes: [
      'f=[1,1]',          // XNN does not support filter height and width of 1.

since you short-circuit 1x1 case, reenable this test?

Copy link
Contributor Author

@annxingyuan annxingyuan 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 @dsmilkov and @Maratyszcza)


tfjs-backend-wasm/src/setup_test.ts, line 42 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

since you short-circuit 1x1 case, reenable this test?

There is still a f=[1,1] test that does not pass the short circuiting condition.


tfjs-backend-wasm/src/cc/kernels/MaxPool.cc, line 83 at r1 (raw file):

Previously, Maratyszcza (Marat Dukhan) wrote…

If xnn_create_max_pooling2d_nhwc_f32 fails, the function should exit early, because max_pool_op is nullptr, and it is not safe to call xnn_setup_max_pooling2d_nhwc_f32 and xnn_run_operator with a nullptr

Done

@annxingyuan annxingyuan merged commit 324c1f1 into master Nov 19, 2019
@annxingyuan annxingyuan deleted the wasm_maxpool branch November 19, 2019 17:01
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.

5 participants