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

[Improve] Migrating DIVUP to GET_BLOCKS #1586

Merged
merged 4 commits into from
Jan 8, 2022
Merged

[Improve] Migrating DIVUP to GET_BLOCKS #1586

merged 4 commits into from
Jan 8, 2022

Conversation

teamwong111
Copy link
Contributor

@teamwong111 teamwong111 commented Dec 14, 2021 •
edited by zhouzaida
Loading

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

resolves #1400
Migrating DIVUP to GET_BLOCKS because GET_BLOCKS looks safer.

Modification

As above.

BC-breaking (Optional)

Does the modification introduce changes that break the backward-compatibility of the downstream repositories?
If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.

Use cases (Optional)

If this PR introduces a new feature, it is better to list some use cases here, and update the documentation.

Checklist

Before PR:

  • I have read and followed the workflow indicated in the CONTRIBUTING.md to create this PR.
  • Pre-commit or linting tools indicated in CONTRIBUTING.md are used to fix the potential lint issues.
  • Bug fixes are covered by unit tests, the case that causes the bug should be added in the unit tests.
  • New functionalities are covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  • The documentation has been modified accordingly, including docstring or example tutorials.

After PR:

  • If the modification has potential influence on downstream or other related projects, this PR should be tested with some of those projects, like MMDet or MMCls.
  • CLA has been signed and all committers have signed the CLA in this PR.

@teamwong111 teamwong111 linked an issue Dec 14, 2021 that may be closed by this pull request
Suggest migrating all DIVUP to GET_BLOCKS as they essentially do the same thing but GET_BLOCKS looks safer. #1400
Closed
@zhouzaida
Copy link
Collaborator

Please @AllentDan have a look.

AllentDan
AllentDan reviewed Dec 15, 2021
View reviewed changes
Copy link
Member

@AllentDan AllentDan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Directly replace DIVUP with GET_BLOCKS may need kernel loop then

mmcv/ops/csrc/common/cuda/common_cuda_helper.hpp Outdated
@@ -9,10 +9,10 @@

#define THREADS_PER_BLOCK 512

#define DIVUP(m, n) ((m) / (n) + ((m) % (n) > 0))
// #define DIVUP(m, n) ((m) / (n) + ((m) % (n) > 0))
Copy link
Member

Choose a reason for hiding this comment

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

can be removed if useless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

mmcv/ops/csrc/parrots/iou3d.cpp Outdated
@@ -134,7 +134,7 @@ void iou3d_nms_forward(Tensor boxes, Tensor keep, Tensor keep_num,
int64_t *keep_data = keep.data_ptr<int64_t>();
int64_t *keep_num_data = keep_num.data_ptr<int64_t>();

const int col_blocks = DIVUP(boxes_num, THREADS_PER_BLOCK_NMS);
const int col_blocks = GET_BLOCKS(boxes_num, THREADS_PER_BLOCK_NMS);
Copy link
Member

Choose a reason for hiding this comment

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

what if actual col_blocks is greater than 4096?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it may be unsafe and we should only use GET_BLOCKS for block allocation. I will fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

mmcv/ops/csrc/parrots/iou3d.cpp Outdated
@@ -189,7 +189,7 @@ void iou3d_nms_normal_forward(Tensor boxes, Tensor keep, Tensor keep_num,
int64_t *keep_data = keep.data_ptr<int64_t>();
int64_t *keep_num_data = keep_num.data_ptr<int64_t>();

const int col_blocks = DIVUP(boxes_num, THREADS_PER_BLOCK_NMS);
const int col_blocks = GET_BLOCKS(boxes_num, THREADS_PER_BLOCK_NMS);
Copy link
Member

Choose a reason for hiding this comment

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

what if actual col_blocks is greater than 4096?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above

AllentDan
AllentDan approved these changes Dec 23, 2021
View reviewed changes
Copy link
Member

@AllentDan AllentDan left a comment

Choose a reason for hiding this comment

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

LGTM after removing useless lines

mmcv/ops/csrc/common/cuda/iou3d_cuda_kernel.cuh
const int blocks =
(boxes_num + THREADS_PER_BLOCK_NMS - 1) / THREADS_PER_BLOCK_NMS;
CUDA_2D_KERNEL_BLOCK_LOOP(col_start, blocks, row_start, blocks) {
// if (row_start > col_start) return;
Copy link
Member

Choose a reason for hiding this comment

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

may remove if useless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This annotation was left before. Shall we delete it? And this statement seems like https://github.com/open-mmlab/mmcv/blame/master/mmcv/ops/csrc/common/cuda/nms_cuda_kernel.cuh#L37, I don't know which is more suitable.

Copy link
Member

Choose a reason for hiding this comment

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

Well, that line seems to depend on how the block is initialized. It is useful when the block is a square or a wide rectangle. As we don't know how the user may initialize the block. Just keep it commented then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get it.

mmcv/ops/csrc/common/cuda/iou3d_cuda_kernel.cuh
const int blocks =
(boxes_num + THREADS_PER_BLOCK_NMS - 1) / THREADS_PER_BLOCK_NMS;
CUDA_2D_KERNEL_BLOCK_LOOP(col_start, blocks, row_start, blocks) {
// if (row_start > col_start) return;
Copy link
Member

Choose a reason for hiding this comment

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

may remove if useless

@ZwwWayne
Copy link
Collaborator

Need to resolve conflicts.

@teamwong111
Copy link
Contributor Author

Need to resolve conflicts.

Done.

grimoire
grimoire approved these changes Dec 30, 2021
View reviewed changes
Copy link
Member

@grimoire grimoire left a comment

Choose a reason for hiding this comment

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

LGTM

@zhouzaida zhouzaida merged commit b586cc2 into open-mmlab:master Jan 8, 2022
@teamwong111 teamwong111 deleted the fix-block-alloc branch January 10, 2022 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AllentDan AllentDan AllentDan approved these changes

@zhouzaida zhouzaida zhouzaida approved these changes

@grimoire grimoire grimoire approved these changes

@ZwwWayne ZwwWayne Awaiting requested review from ZwwWayne

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Suggest migrating all DIVUP to GET_BLOCKS as they essentially do the same thing but GET_BLOCKS looks safer.
5 participants
@teamwong111 @zhouzaida @ZwwWayne @grimoire @AllentDan