Skip to content

Conversation

@xhcao
Copy link
Contributor

@xhcao xhcao commented Feb 22, 2023

This patch supports AvgPool3D, MaxPool3D and
MaxPool3DGrad kernels.

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 and removed request for gyagp February 22, 2023 08:35
const convInfo = backend_util.computePool3DInfo(
x.shape as [number, number, number, number, number], filterSize, strides,
dilations, pad, dimRoundingMode, dataFormat);
const maxPoolProgram = new Pool3DProgram(convInfo, 'avg');
Copy link
Contributor

Choose a reason for hiding this comment

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

maxPoolProgram -> avgPoolProgram or program

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{
type: 'int32',
data: [
convInfo.dilationDepth, convInfo.dilationHeight, convInfo.dilationWidth
Copy link
Contributor

Choose a reason for hiding this comment

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

In L30, you always use dilations = [1, 1, 1]. Do we still need to pass it to uniform? And I see the API doc also doesn't have this parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Thanks for finding there is no dilations for 3d pool kernels.

},
{
type: 'int32',
data: [convInfo.inDepth, convInfo.inHeight, convInfo.inWidth]
Copy link
Contributor

Choose a reason for hiding this comment

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

reuse the uniforms.xShape?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although it doesn't support channelsFirst now, the convInfo.inDepth, convInfo.inHeight and convInfo.inWidth had handled the dataFormat attribute, if we use uniforms.xShape, we will also need handle the dataFormat attribute in the shader again.
If we decide that we must reuse the uniforms.xShape, we could modify all related code in the future, not only one here.

}
let idyR = i32(dyR);
for (var wC = 0; wC < uniforms.filterDims[2]; wC++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For wD and wR, you both add the dilation, if dilations are needed, please add it here to keep consistent. Otherwise, remove all dilations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

This patch supports AvgPool3D, MaxPool3D and
MaxPool3DGrad kernels.
dispatch: [number, number, number];
variableNames = ['dy', 'maxPos'];
uniforms =
`strides : vec3<i32>, pads : vec3<i32>, dilations : vec3<i32>, filterDims : vec3<i32>,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: dilations should be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Had forgotten to remove dilations information for MaxPool3DGrad kernel. Thanks.

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

@qjia7 qjia7 merged commit e26e40d into tensorflow:master Feb 28, 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