Skip to content

[webgpu] Fix wrongly used u32 of WGSL - #5559

Merged
lina128 merged 2 commits into
tensorflow:masterfrom
axinging:fix_u32_wgsl
Sep 1, 2021
Merged

[webgpu] Fix wrongly used u32 of WGSL#5559
lina128 merged 2 commits into
tensorflow:masterfrom
axinging:fix_u32_wgsl

Conversation

@axinging

@axinging axinging commented Aug 31, 2021

Copy link
Copy Markdown
Contributor

With this change, when the real coord < 0, 0.0 will be returned. However, without this change, when the real coord < 0, 0.0 will also be returned. This is why existing case can not catch this error.

Below is explanation for without this change. For example:

// Before this PR, coordCol  is of type u32.
let coordCol = outCol * uniforms.stride[1] + uniforms.dilation[1] * WCol - uniforms.pad[1];

When the real result of right side < 0, coordCol (of type u32) will be interpreted as a very big data, this big data will fail the check done by coordsInBounds4D, so 0.0 will be returned.


This change is Reviewable

@axinging axinging changed the title [DRAFT] Fix wrongly used u32 [webgpu] Fix wrongly used u32 Aug 31, 2021
@axinging

Copy link
Copy Markdown
Contributor Author

@qjia7 @haoyunfeix @xhcao @gyagp , PTAL

@axinging axinging changed the title [webgpu] Fix wrongly used u32 [webgpu] Fix wrongly used u32 of WGSL Aug 31, 2021
@qjia7
qjia7 requested review from kainino0x and lina128 August 31, 2021 12:34
@qjia7

qjia7 commented Aug 31, 2021

Copy link
Copy Markdown
Contributor

@kainino0x @lina128 take a look, thanks.

@lina128 lina128 left a comment

Copy link
Copy Markdown
Collaborator

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 @axinging and @kainino0x)


tfjs-backend-webgpu/src/kernels/conv2d_mm_vec4_webgpu.ts, line 312 at r1 (raw file):

 if (coordRow < 0) {

If coordRow < 0, is this an error? Why handle it silently?

@axinging

axinging commented Sep 1, 2021

Copy link
Copy Markdown
Contributor Author

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

tfjs-backend-webgpu/src/kernels/conv2d_mm_vec4_webgpu.ts, line 312 at r1 (raw file):

 if (coordRow < 0) {

If coordRow < 0, is this an error? Why handle it silently?

coordRow < 0 happens when padding is applied during computing convolution. This is intended behavior, can not report error.

@lina128 lina128 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 3 of 1 approvals obtained (waiting on @axinging)


tfjs-backend-webgpu/src/kernels/conv2d_mm_vec4_webgpu.ts, line 312 at r1 (raw file):

Previously, axinging (Xu Xing) wrote…

coordRow < 0 happens when padding is applied during computing convolution. This is intended behavior, can not report error.

Ah, right. If it's uint, then it will get a large number, which is incorrect. Got it. Thanks.

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.

4 participants