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

[ir] [transforms] Added assertion that indices won't cause overflow under debug mode #2199

Merged
merged 8 commits into from
Mar 13, 2021

Conversation

Leonz5288
Copy link
Contributor

Related issue = #2146

With large indices such as variable[300][300][300][300], when performing IR transform and simplifying this to an integer, this integer will overflow to negative number, which would cause SegFault. I added assertion to IR under debug mode which would assert that the simplified index must NOT be less than 0. One test is provided named "test_indices_assert.py".

For modification in docs, there is one typo that added an 's' after 'exclude'. I simply deleted this 's'.
@k-ye

[Click here for the format server]


@k-ye k-ye self-requested a review March 3, 2021 23:35
@k-ye k-ye changed the title [ir] [transforms] [docs] Added assertion that indices won't cause overflow under debug mode. Fixed one typo in write_test docs. [ir] [transforms] Added assertion that indices won't cause overflow under debug mode Mar 3, 2021
Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

auto zero = Stmt::make<ConstStmt>(LaneAttribute<TypedConstant>(0));
auto check_sum = Stmt::make<BinaryOpStmt>(BinaryOpType::cmp_ge, sum.get(), zero.get());
auto assert = Stmt::make<AssertStmt>(check_sum.get(), "The indices provided are too big!", std::vector<Stmt *>());
auto select = Stmt::make<TernaryOpStmt>(TernaryOpType::select, check_sum.get(), sum.get(), zero.get());
Copy link
Member

Choose a reason for hiding this comment

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

Let's add this comment to explain why we need select:

Because Taichi's assertion is checked only after the execution of the whole kernel, when the linear index overflows and goes negative, we have to replace that with 0 to make sure that the rest of the kernel can still complete. Otherwise, Taichi would crash due to the illegal memory address.

tests/python/test_indices_assert.py Outdated Show resolved Hide resolved
tests/python/test_indices_assert.py Outdated Show resolved Hide resolved
Co-authored-by: Ye Kuang <k-ye@users.noreply.github.com>
@Leonz5288 Leonz5288 requested a review from k-ye March 13, 2021 02:16
@k-ye k-ye requested a review from taichi-gardener March 13, 2021 02:17
Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

LGTM

@k-ye k-ye changed the title [ir] [transforms] Added assertion that indices won't cause overflow under debug mode [ir] [transforms] Added assertion that indices won't cause overflow under debug mode Mar 13, 2021
@k-ye k-ye merged commit 13751c5 into taichi-dev:master Mar 13, 2021
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