Skip to content

Conversation

copyrightly
Copy link
Contributor

Summary:
The existing implementation of aten::sum.dim_IntList depends on the following steps:

  • store the items of arguments opt_dim in a std::set<int64_t> dims_set;
  • iterate through dims_set in the reverse order(i.e. from largest to smallest) and compute the sum for one designated dim in sum_dim

But when opt_dim contains negative items and keepdim==false, the dimension iteration over the set will be messed up. For example, the existing implementation will fail at the test case test_sum_dim({10, 7, 5}, {-1, -2});.

We fix the issue by invoking int64_t dim_normalized = utils::normalize(d, self.dim()); to get normalized dim in the range [0, self.dim() - 1].

Moreover the existing TORCH_CHECK of the condition

d >= -self.dim() - 1 && d <= self.dim()

is wrong and fixed by

d >= -self.dim() && d < self.dim()

Test Plan:

[luwei@devbig984.prn1 /data/users/luwei/fbsource (04b08a835)]$ LD_LIBRARY_PATH=third-party/swiftshader/lib/linux-x64/ buck run fbcode/mode/dev-nosan //xplat/caffe2:pt_vulkan_api_test_bin -- --gtest_filter="*sum*"
Building: finished in 0.1 sec (100%) 339/339 jobs, 0/339 updated
  Total time: 0.2 sec
BUILD SUCCEEDED
Running main() from third-party/googletest/1.11.0/googletest/googletest/src/gtest_main.cc
Note: Google Test filter = *sum*
[==========] Running 8 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 8 tests from VulkanAPITest
[ RUN      ] VulkanAPITest.cumsum
[       OK ] VulkanAPITest.cumsum (105 ms)
[ RUN      ] VulkanAPITest.sum_invalid_inputs
[       OK ] VulkanAPITest.sum_invalid_inputs (0 ms)
[ RUN      ] VulkanAPITest.sum_dim_2d
[       OK ] VulkanAPITest.sum_dim_2d (145 ms)
[ RUN      ] VulkanAPITest.sum_dim_3d
[       OK ] VulkanAPITest.sum_dim_3d (91 ms)
[ RUN      ] VulkanAPITest.sum_dim_4d
[       OK ] VulkanAPITest.sum_dim_4d (89 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_2d
[       OK ] VulkanAPITest.sum_dim_keepdim_2d (63 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_3d
[       OK ] VulkanAPITest.sum_dim_keepdim_3d (135 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_4d
[       OK ] VulkanAPITest.sum_dim_keepdim_4d (4 ms)
[----------] 8 tests from VulkanAPITest (637 ms total)

[----------] Global test environment tear-down
[==========] 8 tests from 1 test suite ran. (637 ms total)
[  PASSED  ] 8 tests.

Reviewed By: yipjustin

Differential Revision: D50442152

@pytorch-bot pytorch-bot bot added ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR module: vulkan release notes: vulkan release notes category labels Oct 19, 2023
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 19, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/111586

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 02c3949 with merge base 2a40b7e (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

Summary:
The existing implementation of `aten::sum.dim_IntList` depends on the following steps:
- store the items of arguments `opt_dim` in a  `std::set<int64_t> dims_set;`
- iterate through `dims_set` in the reverse order(i.e. from largest to smallest) and compute the sum for one designated dim in `sum_dim`

But when `opt_dim` contains negative items and `keepdim==false`, the dimension iteration over the  set will be messed up. For example, the existing implementation will fail at the test case `test_sum_dim({10, 7, 5}, {-1, -2});`.

We fix the issue by invoking `int64_t dim_normalized = utils::normalize(d, self.dim());` to get normalized dim in the range [0, `self.dim()` - 1].

Moreover the existing TORCH_CHECK of the condition
```
d >= -self.dim() - 1 && d <= self.dim()
```
is wrong and fixed by
```
d >= -self.dim() && d < self.dim()
```

Test Plan:
```
[luwei@devbig984.prn1 /data/users/luwei/fbsource (04b08a835)]$ LD_LIBRARY_PATH=third-party/swiftshader/lib/linux-x64/ buck run fbcode/mode/dev-nosan //xplat/caffe2:pt_vulkan_api_test_bin -- --gtest_filter="*sum*"
Building: finished in 0.1 sec (100%) 339/339 jobs, 0/339 updated
  Total time: 0.2 sec
BUILD SUCCEEDED
Running main() from third-party/googletest/1.11.0/googletest/googletest/src/gtest_main.cc
Note: Google Test filter = *sum*
[==========] Running 8 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 8 tests from VulkanAPITest
[ RUN      ] VulkanAPITest.cumsum
[       OK ] VulkanAPITest.cumsum (105 ms)
[ RUN      ] VulkanAPITest.sum_invalid_inputs
[       OK ] VulkanAPITest.sum_invalid_inputs (0 ms)
[ RUN      ] VulkanAPITest.sum_dim_2d
[       OK ] VulkanAPITest.sum_dim_2d (145 ms)
[ RUN      ] VulkanAPITest.sum_dim_3d
[       OK ] VulkanAPITest.sum_dim_3d (91 ms)
[ RUN      ] VulkanAPITest.sum_dim_4d
[       OK ] VulkanAPITest.sum_dim_4d (89 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_2d
[       OK ] VulkanAPITest.sum_dim_keepdim_2d (63 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_3d
[       OK ] VulkanAPITest.sum_dim_keepdim_3d (135 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_4d
[       OK ] VulkanAPITest.sum_dim_keepdim_4d (4 ms)
[----------] 8 tests from VulkanAPITest (637 ms total)

[----------] Global test environment tear-down
[==========] 8 tests from 1 test suite ran. (637 ms total)
[  PASSED  ] 8 tests.
```

Reviewed By: yipjustin

Differential Revision: D50442152

fbshipit-source-id: 85a671a405b64725edd2d89c7dce56ff3ef68e51
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D50442152

facebook-github-bot pushed a commit that referenced this pull request Oct 19, 2023
…111586)

Summary:

The existing implementation of `aten::sum.dim_IntList` depends on the following steps:
- store the items of arguments `opt_dim` in a  `std::set<int64_t> dims_set;`
- iterate through `dims_set` in the reverse order(i.e. from largest to smallest) and compute the sum for one designated dim in `sum_dim`

But when `opt_dim` contains negative items and `keepdim==false`, the dimension iteration over the  set will be messed up. For example, the existing implementation will fail at the test case `test_sum_dim({10, 7, 5}, {-1, -2});`.

We fix the issue by invoking `int64_t dim_normalized = utils::normalize(d, self.dim());` to get normalized dim in the range [0, `self.dim()` - 1].

Moreover the existing TORCH_CHECK of the condition
```
d >= -self.dim() - 1 && d <= self.dim()
```
is wrong and fixed by
```
d >= -self.dim() && d < self.dim()
```

Test Plan:
```
[luwei@devbig984.prn1 /data/users/luwei/fbsource (04b08a835)]$ LD_LIBRARY_PATH=third-party/swiftshader/lib/linux-x64/ buck run fbcode/mode/dev-nosan //xplat/caffe2:pt_vulkan_api_test_bin -- --gtest_filter="*sum*"
Building: finished in 0.1 sec (100%) 339/339 jobs, 0/339 updated
  Total time: 0.2 sec
BUILD SUCCEEDED
Running main() from third-party/googletest/1.11.0/googletest/googletest/src/gtest_main.cc
Note: Google Test filter = *sum*
[==========] Running 8 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 8 tests from VulkanAPITest
[ RUN      ] VulkanAPITest.cumsum
[       OK ] VulkanAPITest.cumsum (105 ms)
[ RUN      ] VulkanAPITest.sum_invalid_inputs
[       OK ] VulkanAPITest.sum_invalid_inputs (0 ms)
[ RUN      ] VulkanAPITest.sum_dim_2d
[       OK ] VulkanAPITest.sum_dim_2d (145 ms)
[ RUN      ] VulkanAPITest.sum_dim_3d
[       OK ] VulkanAPITest.sum_dim_3d (91 ms)
[ RUN      ] VulkanAPITest.sum_dim_4d
[       OK ] VulkanAPITest.sum_dim_4d (89 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_2d
[       OK ] VulkanAPITest.sum_dim_keepdim_2d (63 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_3d
[       OK ] VulkanAPITest.sum_dim_keepdim_3d (135 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_4d
[       OK ] VulkanAPITest.sum_dim_keepdim_4d (4 ms)
[----------] 8 tests from VulkanAPITest (637 ms total)

[----------] Global test environment tear-down
[==========] 8 tests from 1 test suite ran. (637 ms total)
[  PASSED  ] 8 tests.
```

Reviewed By: yipjustin

Differential Revision: D50442152
copyrightly added a commit that referenced this pull request Oct 20, 2023
…111586)

Summary:

The existing implementation of `aten::sum.dim_IntList` depends on the following steps:
- store the items of arguments `opt_dim` in a  `std::set<int64_t> dims_set;`
- iterate through `dims_set` in the reverse order(i.e. from largest to smallest) and compute the sum for one designated dim in `sum_dim`

But when `opt_dim` contains negative items and `keepdim==false`, the dimension iteration over the  set will be messed up. For example, the existing implementation will fail at the test case `test_sum_dim({10, 7, 5}, {-1, -2});`.

We fix the issue by invoking `int64_t dim_normalized = utils::normalize(d, self.dim());` to get normalized dim in the range [0, `self.dim()` - 1].

Moreover the existing TORCH_CHECK of the condition
```
d >= -self.dim() - 1 && d <= self.dim()
```
is wrong and fixed by
```
d >= -self.dim() && d < self.dim()
```

Test Plan:
```
[luwei@devbig984.prn1 /data/users/luwei/fbsource (04b08a835)]$ LD_LIBRARY_PATH=third-party/swiftshader/lib/linux-x64/ buck run fbcode/mode/dev-nosan //xplat/caffe2:pt_vulkan_api_test_bin -- --gtest_filter="*sum*"
Building: finished in 0.1 sec (100%) 339/339 jobs, 0/339 updated
  Total time: 0.2 sec
BUILD SUCCEEDED
Running main() from third-party/googletest/1.11.0/googletest/googletest/src/gtest_main.cc
Note: Google Test filter = *sum*
[==========] Running 8 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 8 tests from VulkanAPITest
[ RUN      ] VulkanAPITest.cumsum
[       OK ] VulkanAPITest.cumsum (105 ms)
[ RUN      ] VulkanAPITest.sum_invalid_inputs
[       OK ] VulkanAPITest.sum_invalid_inputs (0 ms)
[ RUN      ] VulkanAPITest.sum_dim_2d
[       OK ] VulkanAPITest.sum_dim_2d (145 ms)
[ RUN      ] VulkanAPITest.sum_dim_3d
[       OK ] VulkanAPITest.sum_dim_3d (91 ms)
[ RUN      ] VulkanAPITest.sum_dim_4d
[       OK ] VulkanAPITest.sum_dim_4d (89 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_2d
[       OK ] VulkanAPITest.sum_dim_keepdim_2d (63 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_3d
[       OK ] VulkanAPITest.sum_dim_keepdim_3d (135 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_4d
[       OK ] VulkanAPITest.sum_dim_keepdim_4d (4 ms)
[----------] 8 tests from VulkanAPITest (637 ms total)

[----------] Global test environment tear-down
[==========] 8 tests from 1 test suite ran. (637 ms total)
[  PASSED  ] 8 tests.
```

Reviewed By: yipjustin

Differential Revision: D50442152
@facebook-github-bot
Copy link
Contributor

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 20, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@facebook-github-bot facebook-github-bot deleted the export-D50442152 branch October 24, 2023 14:23
xuhancn pushed a commit to xuhancn/pytorch that referenced this pull request Nov 7, 2023
…ytorch#111586)

Summary:
The existing implementation of `aten::sum.dim_IntList` depends on the following steps:
- store the items of arguments `opt_dim` in a  `std::set<int64_t> dims_set;`
- iterate through `dims_set` in the reverse order(i.e. from largest to smallest) and compute the sum for one designated dim in `sum_dim`

But when `opt_dim` contains negative items and `keepdim==false`, the dimension iteration over the  set will be messed up. For example, the existing implementation will fail at the test case `test_sum_dim({10, 7, 5}, {-1, -2});`.

We fix the issue by invoking `int64_t dim_normalized = utils::normalize(d, self.dim());` to get normalized dim in the range [0, `self.dim()` - 1].

Moreover the existing TORCH_CHECK of the condition
```
d >= -self.dim() - 1 && d <= self.dim()
```
is wrong and fixed by
```
d >= -self.dim() && d < self.dim()
```

Test Plan:
```
[luwei@devbig984.prn1 /data/users/luwei/fbsource (04b08a835)]$ LD_LIBRARY_PATH=third-party/swiftshader/lib/linux-x64/ buck run fbcode/mode/dev-nosan //xplat/caffe2:pt_vulkan_api_test_bin -- --gtest_filter="*sum*"
Building: finished in 0.1 sec (100%) 339/339 jobs, 0/339 updated
  Total time: 0.2 sec
BUILD SUCCEEDED
Running main() from third-party/googletest/1.11.0/googletest/googletest/src/gtest_main.cc
Note: Google Test filter = *sum*
[==========] Running 8 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 8 tests from VulkanAPITest
[ RUN      ] VulkanAPITest.cumsum
[       OK ] VulkanAPITest.cumsum (105 ms)
[ RUN      ] VulkanAPITest.sum_invalid_inputs
[       OK ] VulkanAPITest.sum_invalid_inputs (0 ms)
[ RUN      ] VulkanAPITest.sum_dim_2d
[       OK ] VulkanAPITest.sum_dim_2d (145 ms)
[ RUN      ] VulkanAPITest.sum_dim_3d
[       OK ] VulkanAPITest.sum_dim_3d (91 ms)
[ RUN      ] VulkanAPITest.sum_dim_4d
[       OK ] VulkanAPITest.sum_dim_4d (89 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_2d
[       OK ] VulkanAPITest.sum_dim_keepdim_2d (63 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_3d
[       OK ] VulkanAPITest.sum_dim_keepdim_3d (135 ms)
[ RUN      ] VulkanAPITest.sum_dim_keepdim_4d
[       OK ] VulkanAPITest.sum_dim_keepdim_4d (4 ms)
[----------] 8 tests from VulkanAPITest (637 ms total)

[----------] Global test environment tear-down
[==========] 8 tests from 1 test suite ran. (637 ms total)
[  PASSED  ] 8 tests.
```

Reviewed By: yipjustin

Differential Revision: D50442152

Pull Request resolved: pytorch#111586
Approved by: https://github.com/yipjustin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR ciflow/trunk Trigger trunk jobs on your pull request fb-exported Merged module: vulkan release notes: vulkan release notes category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants