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] Fixed div by zero core dump. Fixed rounding intrinsics on int crash #5026

Merged
merged 1 commit into from
Mar 12, 2020
Merged

Conversation

dpankratz
Copy link
Contributor

Div by zero crash

I found and fixed 2 instances where a divide by zero could be evaluated in const_fold causing a Python core dump. For example tir.const(2) % 0.

Rounding intrinsics

When I tried to use llvm intrinsics on integers I found all of them but popcount result in an error analogous to:

TVMError: LLVM module verification failed with the following errors: 
Intrinsic has incorrect return type!
i32 (i32)* @llvm.floor.i32

To fix this for trunc,floor,ceil,nearbyint,round it is sufficient to return the int expr and avoid the call altogether.

Other intrinsics

I was looking at fixing the above crash for other intrinsics (sin,cos,atan,tan,tanh,sigmoid,log,erf,exp,sqrt,rsqrt) by restricting their usage to floating point numbers. However applying exp and sqrt to bool type seems to be relied on in this test:

https://github.com/apache/incubator-tvm/blob/master/topi/tests/python/test_topi_reduce.py#L51

Evaluating this line for a bool is equivalent to a tautology which makes me suspect it is not intended. If the reviewer(s) agree that these intrinsics should not be applied to ints, then I can add a front-end checks on the instrinsics to this PR and fix the tests.

I would appreciate a review!

@tqchen

@tqchen
Copy link
Member

tqchen commented Mar 11, 2020

LTGM, @dpankratz can you please rebase against the master

@tqchen
Copy link
Member

tqchen commented Mar 11, 2020

seems that the rebase mess up with the commit history someone, can you try to re-apply the changes on top of the master? Try

git rebase --onto upstream/master HEAD~1

This takes your last commit and rebase it on top of the master

@dpankratz
Copy link
Contributor Author

@tqchen Let me know if that looks good. Thanks for your advice with that command!

@tqchen tqchen merged commit 173b4fc into apache:master Mar 12, 2020
@tqchen
Copy link
Member

tqchen commented Mar 12, 2020

Thanks @dpankratz !

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.

2 participants