[vulkan] Fix warnings: static_cast, remove unused by IvanKobzarev · Pull Request #42195 · pytorch/pytorch

IvanKobzarev · 2020-07-28T21:55:03Z

Stack from ghstack:

[vulkan] Fix warnings: static_cast, remove unused #42195 [vulkan] Fix warnings: static_cast, remove unused

Differential Revision: D22803035

Fix warnings for internal BUCK build that has stricter compilation flags (in comparison with oss build).

static_cast<int32_t> where int64_t sizes, strides are used in Vulkan shaders as int32_t
unused variables in vulkan_test.cpp

[ghstack-poisoned]

ghstack-source-id: d762473 Pull Request resolved: #42195

dr-ci · 2020-07-28T22:02:16Z

💊 CI failures summary and remediations

As of commit 760a8ed (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

Failed: pr/caffe2-pytorch-linux-xenial-rocm3.5.1-py3.6-test

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

AshkanAliabadi · 2020-08-03T23:38:02Z

  };
-  ConstBlock constBlock{image.w(), image.h()};
+  ConstBlock constBlock{static_cast<int32_t>(image.w()),
+                        static_cast<int32_t>(image.h())};


I think safe_downcast is a better choice here but that might requires some refactoring to move it out of Pool.h.

Differential Revision: [D22803035](https://our.internmc.facebook.com/intern/diff/D22803035) Fix warnings for internal BUCK build that has stricter compilation flags (in comparison with oss build). - static_cast<int32_t> where int64_t sizes, strides are used in Vulkan shaders as int32_t - unused variables in `vulkan_test.cpp` [ghstack-poisoned]

ghstack-source-id: 234686d Pull Request resolved: #42195

Differential Revision: [D22803035](https://our.internmc.facebook.com/intern/diff/D22803035) Fix warnings for internal BUCK build that has stricter compilation flags (in comparison with oss build). - static_cast<int32_t> where int64_t sizes, strides are used in Vulkan shaders as int32_t - unused variables in `vulkan_test.cpp` [ghstack-poisoned]

ghstack-source-id: 465d0ae Pull Request resolved: #42195

Differential Revision: [D22803035](https://our.internmc.facebook.com/intern/diff/D22803035) Fix warnings for internal BUCK build that has stricter compilation flags (in comparison with oss build). - static_cast<int32_t> where int64_t sizes, strides are used in Vulkan shaders as int32_t - unused variables in `vulkan_test.cpp` [ghstack-poisoned]

ghstack-source-id: 061052d Pull Request resolved: #42195

AshkanAliabadi · 2020-08-06T23:01:03Z


+namespace detail {
+template <typename To, typename From>
+inline constexpr To safe_downcast(From v) {


I'm not sure this can be constexpr anymore considering that some implementations of TORCH_CHECK might throw so if you run into any compilation issues, that's a potential reason.

Also please mark v as const.

AshkanAliabadi · 2020-08-06T23:08:52Z

+  constexpr Type max{static_cast<Type>(std::numeric_limits<To>::max())};
+  TORCH_CHECK(min <= v && v <= max, "Cast failed: out of range");
+  return static_cast<To>(v);
+}


This function should probably be in c10 since it is general purpose. You can talk with David, Dima, and c10 maintainers if you are OK with moving it there.

AshkanAliabadi · 2020-08-06T23:12:17Z

    int64_t KW) {
-  return {{ALIGN_UP4(C), UP_DIV(OC, 4), KH * KW},
-          {ALIGN_UP4(C), UP_DIV(OC, 4), KH * KW}};
+  const uint32_t Cup4 = ALIGN_UP4(static_cast<uint32_t>(C));


I should have mentioned this earlier, but inline functions (and functions in general) are superior to macros. A change you can consider in a later PR.

AshkanAliabadi · 2020-08-06T23:12:42Z

  constBuffer.bind(descriptorSet, 4);

-  WorkGroupSize workGroupSize{1, 1, params.OC_4};
+  WorkGroupSize workGroupSize{1, 1, static_cast<uint32_t>(params.OC_4)};


safe_downcast?

AshkanAliabadi · 2020-08-06T23:13:03Z

-  return {{ALIGN_UP4(C), UP_DIV(OC, 4), KH * KW},
-          {ALIGN_UP4(C), UP_DIV(OC, 4), KH * KW}};
+  const uint32_t Cup4 = ALIGN_UP4(static_cast<uint32_t>(C));
+  const uint32_t OC_4 = UP_DIV(static_cast<uint32_t>(OC), 4);


safe_downcast?

AshkanAliabadi · 2020-08-06T23:13:09Z

-          {ALIGN_UP4(C), UP_DIV(OC, 4), KH * KW}};
+  const uint32_t Cup4 = ALIGN_UP4(static_cast<uint32_t>(C));
+  const uint32_t OC_4 = UP_DIV(static_cast<uint32_t>(OC), 4);
+  const uint32_t Z = static_cast<uint32_t>(KH) * static_cast<uint32_t>(KW);


AshkanAliabadi · 2020-08-06T23:16:40Z

+namespace detail {
+template <typename To, typename From>
+inline constexpr To safe_downcast(From v) {
+  if (std::is_signed<From>::value && std::is_unsigned<To>::value) {


Also this is more of an advanced topic (just a tip - you don't need to make the change), but since this condition is completely a compile-time condition, you could either use SFINAE in pre C++-17 or if constexpr in C++-17 and onward to isolate it. That would also prevent against compilation errors in case this if block contained a block of code that would only make sense (i.e. was only compilable) if this condition held true but not in general. This is not the case here.

I have updated PR with enable_if construction :

template <typename To, typename From> inline constexpr To safe_downcast_internal(From v) { typedef std::common_type_t<From, To> Type; constexpr Type min{static_cast<Type>(std::numeric_limits<To>::lowest())}; const Type value{static_cast<Type>(v)}; constexpr Type max{static_cast<Type>(std::numeric_limits<To>::max())}; TORCH_CHECK(min <= v && v <= max, "Cast failed: out of range"); return static_cast<To>(v); } template <typename To, typename From> inline constexpr bool IsSignedToUnsigned() { return std::is_signed<From>::value && std::is_unsigned<To>::value; } template < typename To, typename From, std::enable_if_t<IsSignedToUnsigned<To, From>(), bool> = true> inline constexpr To safe_downcast(From v) { TORCH_CHECK(v >= From{}, "Cast failed: negative signed to unsigned"); return safe_downcast_internal<To, From>(v); } template < typename To, typename From, std::enable_if_t<!IsSignedToUnsigned<To, From>(), bool> = true> inline constexpr To safe_downcast(From v) { return safe_downcast_internal<To, From>(v); }

AshkanAliabadi · 2020-08-06T23:31:54Z

By the way, I also suggest calling this numeric_cast. Downcast implies some sort of inheritance hierarchy.

Differential Revision: [D22803035](https://our.internmc.facebook.com/intern/diff/D22803035) Fix warnings for internal BUCK build that has stricter compilation flags (in comparison with oss build). - static_cast<int32_t> where int64_t sizes, strides are used in Vulkan shaders as int32_t - unused variables in `vulkan_test.cpp` [ghstack-poisoned]

ghstack-source-id: 331f49e Pull Request resolved: #42195

Differential Revision: [D22803035](https://our.internmc.facebook.com/intern/diff/D22803035) Fix warnings for internal BUCK build that has stricter compilation flags (in comparison with oss build). - static_cast<int32_t> where int64_t sizes, strides are used in Vulkan shaders as int32_t - unused variables in `vulkan_test.cpp` [ghstack-poisoned]

ghstack-source-id: 0f6b42d Pull Request resolved: #42195

facebook-github-bot · 2020-08-07T19:42:26Z

@IvanKobzarev merged this pull request in 04c62d4.

Summary: Pull Request resolved: pytorch#42195 Test Plan: Imported from OSS Reviewed By: AshkanAliabadi Differential Revision: D22803035 Pulled By: IvanKobzarev fbshipit-source-id: d7bf256437eccb5c421a7fd0aa8ec23a8fec0470

[vulkan] Fix warnings: static_cast, remove unused

9bb0235

[ghstack-poisoned]

IvanKobzarev mentioned this pull request Jul 28, 2020

[vulkan] Ops registration to TORCH_LIBRARY_IMPL #42194

Closed

IvanKobzarev added a commit that referenced this pull request Jul 28, 2020

[vulkan] Fix warnings: static_cast, remove unused

10db456

ghstack-source-id: d762473 Pull Request resolved: #42195

IvanKobzarev requested review from AshkanAliabadi and dreiss July 28, 2020 22:31

AshkanAliabadi reviewed Aug 3, 2020

View reviewed changes

IvanKobzarev added a commit that referenced this pull request Aug 4, 2020

[vulkan] Fix warnings: static_cast, remove unused

0920a0d

ghstack-source-id: 234686d Pull Request resolved: #42195

IvanKobzarev added a commit that referenced this pull request Aug 6, 2020

[vulkan] Fix warnings: static_cast, remove unused

8f373a6

ghstack-source-id: 465d0ae Pull Request resolved: #42195

IvanKobzarev requested a review from AshkanAliabadi August 6, 2020 22:39

IvanKobzarev added a commit that referenced this pull request Aug 6, 2020

[vulkan] Fix warnings: static_cast, remove unused

74ef46e

ghstack-source-id: 061052d Pull Request resolved: #42195

AshkanAliabadi reviewed Aug 6, 2020

View reviewed changes

AshkanAliabadi approved these changes Aug 6, 2020

View reviewed changes

IvanKobzarev added a commit that referenced this pull request Aug 7, 2020

[vulkan] Fix warnings: static_cast, remove unused

5aeb68c

ghstack-source-id: 331f49e Pull Request resolved: #42195

IvanKobzarev added a commit that referenced this pull request Aug 7, 2020

[vulkan] Fix warnings: static_cast, remove unused

213d8fd

ghstack-source-id: 0f6b42d Pull Request resolved: #42195

facebook-github-bot closed this in 04c62d4 Aug 7, 2020

facebook-github-bot added the merged label Aug 7, 2020

facebook-github-bot deleted the gh/IvanKobzarev/66/head branch August 11, 2020 14:16

mruberry added the Merged label Oct 28, 2020

Conversation

IvanKobzarev commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci Bot commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AshkanAliabadi commented Aug 6, 2020

Uh oh!

facebook-github-bot commented Aug 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

IvanKobzarev commented Jul 28, 2020 •

edited

Loading

dr-ci Bot commented Jul 28, 2020 •

edited

Loading