webgpu: NaN refactor #7352
webgpu: NaN refactor #7352
Conversation
|
@hujiajie As offline talked with you, please take a look. Thanks. |
| return ${op}(a, b); | ||
| `; | ||
| } | ||
| const ATAN2 = 'var resultTemp = atan2(a, b);'; |
There was a problem hiding this comment.
Ops are ordered alphabetically above.
| return (resultTemp + b) % b; | ||
| } | ||
| resultTemp = select((resultTemp + b) % b, resultTemp, | ||
| (a < 0. && b < 0.) || (a >= 0. && b > 0.)); |
There was a problem hiding this comment.
What's the line breaking rule? Follow clang-format?
| continue; | ||
| } | ||
| const checkNaNSnippet = | ||
| useVec4 ? CHECK_NAN_SNIPPET_VEC4 : CHECK_NAN_SNIPPET; |
There was a problem hiding this comment.
This is a bit strange to have a new variable whose name is the same as one of conditions. Can we just use "useVec4 ? CHECK_NAN_SNIPPET_VEC4 : CHECK_NAN_SNIPPET" directly?
There was a problem hiding this comment.
While it's not the case here, sometimes clang-format behaves poorly on ${expression}. To reiterate my previous point, personally I prefer the ${identifier} style, or macro expansion in terms of C/C++.
There was a problem hiding this comment.
clang-format behaves poorly on ${}, no matter it's expression or identifier. We can now leave it as is.
|
|
||
| export function getBinaryOpString( | ||
| type: BinaryOpType, useVec4?: boolean): string { | ||
| // NaN-sensitive ops |
There was a problem hiding this comment.
Perhaps "sensitive"/"insensitive" is not the right word here, but I'm not sure what could be better.
There was a problem hiding this comment.
Maybe "Ops with NaN check" and "Ops without NaN check"?
qjia7
left a comment
qjia7
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @gyagp, @hujiajie, and @xhcao)
tfjs-backend-webgpu/src/binary_op_util.ts line 114 at r1 (raw file):
Previously, hujiajie (Jiajie Hu) wrote…
What's the line breaking rule? Follow clang-format?
I break it manually since it's too long.
tfjs-backend-webgpu/src/binary_op_util.ts line 187 at r1 (raw file):
Previously, gyagp (Yang Gu) wrote…
Maybe "Ops with NaN check" and "Ops without NaN check"?
Done.
tfjs-backend-webgpu/src/binary_op_util.ts line 213 at r1 (raw file):
Previously, gyagp (Yang Gu) wrote…
This is a bit strange to have a new variable whose name is the same as one of conditions. Can we just use "useVec4 ? CHECK_NAN_SNIPPET_VEC4 : CHECK_NAN_SNIPPET" directly?
Done.
|
Thanks but sorry my brain bandwidth is a little exhausted, since the patch seems to be squeezing several changes into one. I would like to resort to others' feedback. |
4 participants
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is