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

[Bugfix] Nms_ir data_race solved #2600

Merged
merged 3 commits into from
Feb 18, 2019
Merged

[Bugfix] Nms_ir data_race solved #2600

merged 3 commits into from
Feb 18, 2019

Conversation

Laurawly
Copy link
Contributor

Bug reproduced on Mali GPU. Previously was tested on Intel graphics, Nvidia GPUs, worked find with no erros. @vinx13 Please lemme know if you think this also applies to faster-r-cnn, thanks!

@vinx13 @masahi Please help review

@@ -151,8 +151,7 @@ def calculate_overlap(out_tensor, box_a_idx, box_b_idx):
with ib.if_scope(tvm.all(nms_topk_node > 0, nms_topk < p_valid_count[b])):
with ib.for_range(0, p_valid_count[b] - nkeep) as l:
with ib.if_scope(i < 6):
p_out[(base_idx + (l + nkeep) * 6 + i)] = \
p_data[(base_idx + (l + nkeep) * 6 + i)]
p_out[(base_idx + (l + nkeep) * 6 + i)] = -1.0
Copy link
Member

Choose a reason for hiding this comment

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

What's the meaning of -1? Are the last (valid_count - nkeep) bboxes dropped by this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's based on the implementation of the GluonCV SSD, plz reference PR #2353 .

@masahi masahi self-assigned this Feb 15, 2019
@@ -169,6 +168,9 @@ def calculate_overlap(out_tensor, box_a_idx, box_b_idx):
base_idx + offset_i + 2)
with ib.if_scope(iou >= nms_threshold):
p_out[base_idx + offset_i] = -1.0
ib.emit(tvm.make.Call(None, 'tvm_storage_sync',
Copy link
Member

Choose a reason for hiding this comment

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

this makes sense to me, could you also add this to proposal op?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I’ll do that.

@masahi masahi merged commit fef7282 into apache:master Feb 18, 2019
@masahi
Copy link
Member

masahi commented Feb 18, 2019

thanks @Laurawly @vinx13 this is merged.

wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
* nms data race solved

* tst_topi_vision reference results are gonna be updated in PR apache#2353

* proposal nms_ir updated
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
* nms data race solved

* tst_topi_vision reference results are gonna be updated in PR apache#2353

* proposal nms_ir updated
@yzhliu yzhliu mentioned this pull request Mar 2, 2019
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants