Skip to content

Conversation

IvanKobzarev
Copy link
Contributor

@IvanKobzarev IvanKobzarev commented Jul 14, 2020

Stack from ghstack:

Differential Revision: D22754944

[ghstack-poisoned]
@dr-ci
Copy link

dr-ci bot commented Jul 15, 2020

💊 CI failures summary and remediations

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


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed


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.

Copy link
Contributor

@AshkanAliabadi AshkanAliabadi left a comment

Choose a reason for hiding this comment

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

Suggestion: more rigorous const-correctness including shaders.

TORCH_CHECK(
kernel_size.size() == 1 || kernel_size.size() == 2,
"Vulkan max_pool2d: kernel_size must either be a single int, or a tuple of two ints")
const int kH = safe_downcast<int, int64_t>(kernel_size[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This usage is technically correct but template argument deduction can deduce the second template parameter (i.e. source type) passed as a function argument, so this will work as well:

const int kH = safe_downcast<int>(kernel_size[0]); 

self.dim() == 4, "Vulkan max_pool2d is implemented for 4-dim input");

const auto& x = vtensor_from_vulkan(self);
auto inputSize = 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.

Is this const? If so auto does not imply const.

uConstBlock;

#define UP_DIV(x, y) (((x) + (y)-1) / (y))
#define FLT_MAX 3.402823466e+38
Copy link
Contributor

Choose a reason for hiding this comment

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

If these are used in other shaders please consider moving to a common location.

@facebook-github-bot
Copy link
Contributor

@IvanKobzarev merged this pull request in b852168.

@facebook-github-bot facebook-github-bot deleted the gh/IvanKobzarev/57/head branch August 8, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants