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.

Sign up for GitHub

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

Jump to bottom

[vulkan] Add mean.dim op for vulkan #47312

Closed
wants to merge 2 commits into from
Closed

[vulkan] Add mean.dim op for vulkan #47312

wants to merge 2 commits into from

Conversation

SS-JIA
Copy link
Contributor

@SS-JIA SS-JIA commented Nov 3, 2020 •
edited

Stack from ghstack:

Differential Revision: D24713617

@SS-JIA SS-JIA requested review from IvanKobzarev and AshkanAliabadi and removed request for IvanKobzarev November 3, 2020 20:43
AshkanAliabadi
AshkanAliabadi approved these changes Nov 3, 2020
View reviewed changes
aten/src/ATen/native/vulkan/glsl/mean.glsl Outdated
layout(set = 0, binding = 2) uniform constBlock {
layout(set = 0, binding = 0, rgba16f) uniform PRECISION restrict writeonly image3D uOutput;
layout(set = 0, binding = 1) uniform PRECISION sampler3D uInput;
layout(set = 0, binding = 2) uniform Block {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please mark this with PRECISION and restrict.

aten/src/ATen/native/vulkan/glsl/mean.glsl Outdated
for (xi = 0; xi < W; ++xi) {
for (yi = 0; yi < H; ++yi) {
for (xi = 0; xi < uBlock.W; ++xi) {
for (yi = 0; yi < uBlock.H; ++yi) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Iterate over y in the outer loop, and x in the inner loop. We are dealing with a texture that is packed in an opaque format, so this might not apply, but if and when the memory is laid out linearly that traversal has better locality of access.

Copy link
Contributor

Choose a reason for hiding this comment

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

Check adaptive_avg_pool shader here: https://github.com/pytorch/pytorch/pull/47261/files

AshkanAliabadi
AshkanAliabadi reviewed Nov 3, 2020
View reviewed changes
aten/src/ATen/native/vulkan/glsl/mean.glsl Outdated
int OW = uConstBlock.OW;
int OH = uConstBlock.OH;
vec4 r = vec4(1.0) / float(W) / float(H);
vec4 r = vec4(1.0) / float(uBlock.W) / float(uBlock.H);
Copy link
Contributor

Choose a reason for hiding this comment

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

Check adaptive_avg_pool here https://github.com/pytorch/pytorch/pull/47261/files for another implementation. Divisions are typically slower than multiplications.

AshkanAliabadi
AshkanAliabadi reviewed Nov 3, 2020
View reviewed changes
aten/src/ATen/test/vulkan_api_test.cpp Outdated
@@ -169,6 +169,42 @@ TEST(VulkanTest, mm) {
ASSERT_TRUE(check);
}

TEST(VulkanTest, mean) {
Copy link
Contributor

Choose a reason for hiding this comment

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

VulkanAPITest

IvanKobzarev
IvanKobzarev approved these changes Nov 3, 2020
View reviewed changes
aten/src/ATen/test/vulkan_api_test.cpp Outdated

const auto check = almostEqual(t_out, t_out_expected);
if (!check) {
//std::cout << "original:\n" << t_in << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

clean or uncomment?

aten/src/ATen/native/vulkan/glsl/mean2d.glsl Outdated
}
vec4 outValue = r * acc;

int test = (imageSize(uOutput).x*pos.x + pos.x);
Copy link
Contributor

Choose a reason for hiding this comment

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

clean?

@dr-ci
Copy link

dr-ci bot commented Nov 3, 2020 •
edited

💊 CI failures summary and remediations

As of commit 9185eb4 (more details on the Dr. CI page):

  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See CircleCI build pytorch_linux_bionic_py3_8_gcc9_coverage_build (1/1)

Step: "Build" (full log | diagnosis details | 🔁 rerun)

Nov 03 21:38:57 sccache: error: couldn't connect to server 
Nov 03 21:38:57 +++ eval 'extract_trap_cmd ' 
Nov 03 21:38:57 ++++ extract_trap_cmd 
Nov 03 21:38:57 ++++ printf '%s\n' '' 
Nov 03 21:38:57 +++ printf '%s\n' cleanup 
Nov 03 21:38:57 ++ trap -- ' 
Nov 03 21:38:57 cleanup' EXIT 
Nov 03 21:38:57 ++ [[ pytorch-linux-bionic-py3.8-gcc9-coverage-build != *pytorch-win-* ]] 
Nov 03 21:38:57 ++ which sccache 
Nov 03 21:38:57 ++ sccache --stop-server 
Nov 03 21:38:57 Stopping sccache server... 
Nov 03 21:38:57 sccache: error: couldn't connect to server 
Nov 03 21:38:57 sccache: caused by: Connection refused (os error 111) 
Nov 03 21:38:57 ++ true 
Nov 03 21:38:57 ++ rm /var/lib/jenkins/sccache_error.log 
Nov 03 21:38:57 rm: cannot remove '/var/lib/jenkins/sccache_error.log': No such file or directory 
Nov 03 21:38:57 ++ true 
Nov 03 21:38:57 ++ [[ pytorch-linux-bionic-py3.8-gcc9-coverage-build == *rocm* ]] 
Nov 03 21:38:57 ++ SCCACHE_ERROR_LOG=/var/lib/jenkins/sccache_error.log 
Nov 03 21:38:57 ++ SCCACHE_IDLE_TIMEOUT=1200 
Nov 03 21:38:57 ++ RUST_LOG=sccache::server=error 
Nov 03 21:38:57 ++ sccache --start-server
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 4 times.

SS-JIA added a commit that referenced this pull request Nov 3, 2020
@SS-JIA
[vulkan] Add mean.dim op for vulkan
bd86237 
ghstack-source-id: 19a3669e01f721c1ae14b7ea02cd0a901c65ece3
Pull Request resolved: #47312
@facebook-github-bot
Copy link
Contributor

@SS-JIA merged this pull request in 464c569.

@facebook-github-bot facebook-github-bot deleted the gh/SS-JIA/3/head branch November 7, 2020 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AshkanAliabadi AshkanAliabadi AshkanAliabadi approved these changes

@IvanKobzarev IvanKobzarev IvanKobzarev approved these changes

Assignees
No one assigned
Labels
cla signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@SS-JIA @facebook-github-bot @IvanKobzarev @AshkanAliabadi