Skip to content

Conversation

@dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Nov 18, 2019

Call into XNNPACK clamp operator for doing clipping.

NOTE: we can't do operator cleanup upon disposal of a tensor, since there are no weights-like tensors for clamp


This change is Reviewable

Copy link
Contributor

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


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

  const int flags = 0;
  OperatorCacheKey cache_key = {
      static_cast<float>(channels),

Just wondering - what are the implications of not performing the cast?

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: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @Maratyszcza, and @tafsiri)


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

    xnn_status status = xnn_create_clamp_nc_f32(channels, strides, strides, min,
                                                max, flags, &clamp_op);
    if (status != xnn_status_success) {

The function should exit early if xnn_create_clamp_nc_f32 failed. It is not safe to call xnn_setup_clamp_nc_f32 and xnn_run_operator with nullptr for the op

Copy link
Contributor Author

@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.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @Maratyszcza, and @tafsiri)


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

Previously, annxingyuan (Ann Yuan) wrote…

Just wondering - what are the implications of not performing the cast?

compile error (can't auto-cast int to float)


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

Previously, Maratyszcza (Marat Dukhan) wrote…

The function should exit early if xnn_create_clamp_nc_f32 failed. It is not safe to call xnn_setup_clamp_nc_f32 and xnn_run_operator with nullptr for the op

Thanks! #2367 will replace all util::warn calls with throw_js_exception which will result in terminating the program.

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: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @Maratyszcza, and @tafsiri)


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

Previously, dsmilkov (Daniel Smilkov) wrote…

compile error (can't auto-cast int to float)

Why do you use floats as keys? It isn't safe (conversion int->float is lossy, and any NaN would break everything, because (NaN == NaN) == false

Copy link
Contributor Author

@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.

PTAL. I also added unit tests

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @Maratyszcza, and @tafsiri)


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

Previously, Maratyszcza (Marat Dukhan) wrote…

Why do you use floats as keys? It isn't safe (conversion int->float is lossy, and any NaN would break everything, because (NaN == NaN) == false

Because my cache key had a mixture of ints and floats and I didn't know about std::tuple. Changed to std::tuple and removed the casting.

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: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @Maratyszcza, and @tafsiri)


tfjs-backend-wasm/src/cc/kernels/ClipByValue.cc, line 58 at r2 (raw file):

  xnn_operator_t clamp_op = nullptr;
  const int channels = x_info.size;

Since you don't use strides, you can set channels = strides = 1 and batch_size = x_info.size. This will get you much better reuse of xnn_operator_t objects, because channels is a part of the key, but batch_size is not.

Copy link
Contributor Author

@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.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @Maratyszcza, and @tafsiri)


tfjs-backend-wasm/src/cc/kernels/ClipByValue.cc, line 58 at r2 (raw file):

Previously, Maratyszcza (Marat Dukhan) wrote…

Since you don't use strides, you can set channels = strides = 1 and batch_size = x_info.size. This will get you much better reuse of xnn_operator_t objects, because channels is a part of the key, but batch_size is not.

Nice! I reduced the cache key to 2 floats (min & max) since all the other arguments are hard-coded to constants.

@dsmilkov dsmilkov merged commit 48119e9 into master Nov 19, 2019
@dsmilkov dsmilkov deleted the clip branch November 19, 2019 14:46
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