Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

webgpu: Expand DepthwiseConv2DVec4Program to support any stride #6820

Merged
merged 9 commits into from
Sep 15, 2022

Conversation

qjia7
Copy link
Contributor

@qjia7 qjia7 commented Sep 7, 2022

This PR expands DepthwiseConv2DVec4Program's capabilities to support any stride size. Meanwhile,
Constraint stride <= 2 to go to this path.

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


This change is Reviewable

Copy link
Collaborator

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!


// Use constant instead of uniform can give better performance.
for (var wR = 0; wR < ${this.convInfo.filterHeight}; wR = wR + 1) {
let xR = xRCorner + wR;
for (var i = 0; i < ${xNumber}; i++)
if (xR >=0 && xR < uniforms.inDims[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you moves xR's boundary checks out of readX function, should we also move col's boundary check out of readX function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can move xR's boundary checks out of readX is because we are processing the values in the same row. However, we need to check whether each value is out of boundary for col. So col's boundary check is necessary for each value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. I mean, in readX, it may be confusing that we only do boundary check for col while do not for row. Since readX is only used once and it's very straight forward, this LGTM and I was kindly reminding.

Approved.

Copy link

@gyagp gyagp left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@gyagp gyagp left a comment

Choose a reason for hiding this comment

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

In WGSL, we have some format issue. For example, "{" should be in the same line with for and if.

@qjia7 qjia7 merged commit 7423ee7 into tensorflow:master Sep 15, 2022
@qjia7 qjia7 deleted the opt_depthwise_stride2x2 branch September 15, 2022 06:38
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