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

[vulkan_api][ops] Mm, Pool, Upsample #47063

Closed
wants to merge 12 commits into from

Conversation

IvanKobzarev
Copy link
Contributor

@IvanKobzarev IvanKobzarev commented Oct 29, 2020

Stack from ghstack:

Differential Revision: D24624610

mat2_arg.sizes()[1],
mat1_arg.sizes()[0],
1u,
mat1_arg.sizes()[1],
Copy link
Contributor

@SS-JIA SS-JIA Oct 29, 2020

Choose a reason for hiding this comment

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

Should be the last argument? (i.e. corresponds to K in shader Block)

@dr-ci
Copy link

dr-ci bot commented Oct 29, 2020

💊 CI failures summary and remediations

As of commit 5375bb6 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


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 25 times.

IvanKobzarev added a commit that referenced this pull request Oct 29, 2020
ghstack-source-id: c939b3b5c264421288b3c52a0720ac2f62d585d2
Pull Request resolved: #47063

vTensor v_output{
context,
self.sizes(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I changed this to
{mat1.sizes()[0], mat2.sizes()[1]},
and the addmm test is now passing on my laptop

command_buffer.begin();
{
const float scale_x = compute_scales_value<float>(scales_w, input_arg.sizes()[3], output_sizes[2]);
const float scale_y = compute_scales_value<float>(scales_h, input_arg.sizes()[3], output_sizes[2]);
Copy link
Contributor

@SS-JIA SS-JIA Oct 29, 2020

Choose a reason for hiding this comment

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

I think the scales are being computed wrong here (mismatched indices), which is causing the test to fail.

Perhaps should be something like
scale_x = input_arg.sizes()[2]/output_sizes[0]
scale_y = input_arg.sizes()[3]/output_sizes[1]


vTensor v_output{
context,
output_sizes,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since output_sizes only has 2 elements, I also think this should be something like

{input_arg.sizes()[0], input_arg.sizes()[1], output_sizes[0], output_sizes[1]

input_arg.sizes()[3],
input_arg.sizes()[2],
output_sizes[3],
output_sizes[2],
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be output_sizes[1], output_sizes[0] here as well

IvanKobzarev added a commit that referenced this pull request Oct 30, 2020
ghstack-source-id: 13da256a2f788240980fc58a3eceb4612cccc09d
Pull Request resolved: #47063
IvanKobzarev added a commit that referenced this pull request Nov 2, 2020
ghstack-source-id: fee359030ed1bfab2eb613fc7ce3b4d41e0da119
Pull Request resolved: #47063
IvanKobzarev added a commit that referenced this pull request Nov 2, 2020
ghstack-source-id: 3d343b7998d19125b402dfb285544c8578a3d910
Pull Request resolved: #47063
IvanKobzarev added a commit that referenced this pull request Nov 2, 2020
ghstack-source-id: 6a59a73d63c48e9c406e669fab7be8af6dfc1ca0
Pull Request resolved: #47063
@facebook-github-bot
Copy link
Contributor

@IvanKobzarev merged this pull request in 2cff3bb.

@facebook-github-bot facebook-github-bot deleted the gh/ivankobzarev/11/head branch November 6, 2020 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants