Skip to content

Conversation

@hujiajie
Copy link
Contributor

@hujiajie hujiajie commented Dec 23, 2022

This is like

2c95a67 [webgpu] Further tweak vectorized NaN handling in binary ops

but for valueForNaN.

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


This change is Reviewable

This is like

  2c95a67 [webgpu] Further tweak vectorized NaN handling in binary ops

but for `valueForNaN`.
const opFnStr = `
fn binaryOperation(a : ${dType}, b : ${dType}) -> ${dType} {
let isNaN = false;
let valueForNaN = uniforms.NAN;
Copy link

Choose a reason for hiding this comment

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

Currently for NOT_EQUAL_VEC4, valueForNaN is 1.0 instead of uniforms.NAN. Any specific reason? @hujiajie @qjia7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The op returns booleans, even for NaN operands. I think that's by design.

(Well, the doc says nothing about this, it's just explicitly checked in the unit tests. Another mess is that currently this is defined in NOT_EQUAL, but not in EQUAL!)

Copy link

Choose a reason for hiding this comment

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

So do you think if uniforms.NAN is good enough for a default value? I'm not sure if we have more cases like (NOT_)EQUAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see any reason that we use let valueForNaN = 1.0; for NOT_EQUAL_VEC4. For WebGL, there is no NaN check for NotEqual.
So my opinion is we directly use uniforms.NAN in CHECK_NAN_SNIPPET_VEC4. And don't need to define an additional variable valueForNaN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. WGSL implementations may assume that NaNs and infinities are not present at runtime. In such an implementation, when an expression evaluation would produce an infinity or a NaN, an indeterminate value of the target type is produced instead. So, I feel explicit checks are necessary.
  2. We want i32(valueForNaN) to evaluate to 1/true later, but i32(uniforms.NAN) is 0/false.

@qjia7 qjia7 merged commit 5972c3e into tensorflow:master Jan 6, 2023
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