Skip to content

Conversation

@xhcao
Copy link
Contributor

@xhcao xhcao commented Jan 11, 2023

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


This change is Reviewable

@xhcao xhcao requested review from gyagp and qjia7 January 11, 2023 05:30
const maxPoolPositions = backend.runWebGPUProgram(
maxPoolPositionsProgram, [x], 'int32', uniformData);

const avgPoolBackpropProgram = new MaxPool2DBackpropProgram(convInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: avgPoolXXX -> maxPoolXXX

@@ -0,0 +1,93 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Will MaxPool3DBackpropProgram share the same file? If yes, maybe we should remove 2d from file name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, I am not sure the file will be shared by 3d, so we keep the name with 2d here. If we put the 3d in this file, then we could rename the file.

dispatch: [number, number, number];
variableNames = ['dy', 'maxPos'];
uniforms =
`stride : vec2<i32>, pad : vec2<i32>, dilation : vec2<i32>, filterDims : vec2<i32>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we unify to use strides, pads, dilations from this PR? And you can do the refactor for other places in a separate PR.

@xhcao
Copy link
Contributor Author

xhcao commented Jan 16, 2023

@gyagp Please take a look, thanks

this.dispatch = computeDispatch(
this.dispatchLayout, this.outputShape, this.workgroupSize);

this.shaderKey = `max_pool2d_backprop`;
Copy link

Choose a reason for hiding this comment

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

Suggested change
this.shaderKey = `max_pool2d_backprop`;
this.shaderKey = `maxPool2DBackprop`;

What's the convention for shaderKey? Maybe className.replace('Program', '') and variants split by '_'? We need to somehow unify this.

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 with nit to unify shaderKey

@xhcao xhcao merged commit d499b53 into tensorflow:master Jan 17, 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