Skip to content

[webgpu] Fix wrongly used u32 of WGSL#5559

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

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

Conversation

@axinging
Copy link
Copy Markdown
Contributor

@axinging axinging commented Aug 31, 2021

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
Copy link
Copy Markdown
Contributor

qjia7 commented Aug 31, 2021

@kainino0x @lina128 take a look, thanks.

Copy link
Copy Markdown
Collaborator

@lina128 lina128 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 @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
Copy link
Copy Markdown
Contributor Author

axinging commented Sep 1, 2021

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.

Copy link
Copy Markdown
Collaborator

@lina128 lina128 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! 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