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

[SCHEDULE] Fix boundary check #2126

Merged
merged 2 commits into from
Nov 18, 2018
Merged

[SCHEDULE] Fix boundary check #2126

merged 2 commits into from
Nov 18, 2018

Conversation

junrushao
Copy link
Member

@junrushao junrushao commented Nov 18, 2018

Should fix #2088.

Details about this bug are discussed in this comment.

Related issues: #1014, #2088. Related PR: #1091 (which is approximately correct when iv->dom->min == 0).

CC: @tqchen @were.

@were
Copy link
Contributor

were commented Nov 18, 2018

LGTM

@tqchen
Copy link
Member

tqchen commented Nov 18, 2018

Thanks for the contribution, please add a regression test-case https://docs.tvm.ai/contribute/code_review.html#ensure-test-coverage

@tqchen tqchen added the status: need update need update based on feedbacks label Nov 18, 2018
@junrushao
Copy link
Member Author

@tqchen sorry I was thinking that the fix is 2-line, and the correctness should be obvious. Okay I will send an update later today

@tqchen
Copy link
Member

tqchen commented Nov 18, 2018

Just to make sure we don't stupidly change it back again, regression tests are also very useful during code refactors, since we might mistakenly reintroduce the bug again

@junrushao
Copy link
Member Author

@tqchen Sure, I will use the example in our tutorial as the testcase, in which if ((0 <= scan.idx)) should not be there.

@junrushao
Copy link
Member Author

@tqchen added a unittest just now, CI passes, could you help review again? Thanks!

@junrushao junrushao closed this Nov 18, 2018
@junrushao junrushao reopened this Nov 18, 2018
@tqchen tqchen merged commit a376eb3 into apache:master Nov 18, 2018
@tqchen tqchen added status: accepted and removed status: need update need update based on feedbacks labels Nov 18, 2018
FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018
* Fix boundary check

* Add unittest
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
* Fix boundary check

* Add unittest
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
* Fix boundary check

* Add unittest
@junrushao junrushao deleted the fix-boundary-check branch January 6, 2022 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TOPI LSTM recipe fails to compile due to inserting sync is disallowed inside unnecessary IfThenElse
3 participants