You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/07/07 20:55:41 UTC
[GitHub] [tvm] rafzi opened a new pull request, #12034: [TIR] fix crash when comparing IntImm to None
rafzi opened a new pull request, #12034:
URL: https://github.com/apache/tvm/pull/12034
While it is generally preferred to compare to `None` by using `is not`, I don't think TVM should just crash with a segfault in that case.
The crash stack trace is:
https://github.com/apache/tvm/blob/013d5e8fcbd94fb3a0c5c0cdcaea03af43c464aa/python/tvm/tir/expr.py#L568
https://github.com/apache/tvm/blob/013d5e8fcbd94fb3a0c5c0cdcaea03af43c464aa/src/tir/op/op.cc#L984
https://github.com/apache/tvm/blob/013d5e8fcbd94fb3a0c5c0cdcaea03af43c464aa/src/tir/op/op.cc#L514
https://github.com/apache/tvm/blob/013d5e8fcbd94fb3a0c5c0cdcaea03af43c464aa/src/tir/op/op.cc#L102
where rhs is nullptr.
@tqchen @Hzfengsy @wrongtest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 merged pull request #12034: [TIR] fix crash when comparing IntImm to None
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged PR #12034:
URL: https://github.com/apache/tvm/pull/12034
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] rafzi commented on pull request #12034: [TIR] fix crash when comparing IntImm to None
Posted by GitBox <gi...@apache.org>.
rafzi commented on PR #12034:
URL: https://github.com/apache/tvm/pull/12034#issuecomment-1178246916
@junrushao1994 Thank you for your comments. The issue with the asserts is that the comparison would not be supported. It could also add mostly unnecessary code to a hot code path, but I did not profile.
Your suggested change seems more appropriate to me.
Any other opinions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a diff in pull request #12034: [TIR] fix crash when comparing IntImm to None
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12034:
URL: https://github.com/apache/tvm/pull/12034#discussion_r916278184
##########
python/tvm/tir/expr.py:
##########
@@ -562,9 +562,13 @@ def __nonzero__(self):
return self.value != 0
def __eq__(self, other):
+ if not isinstance(other, IntImm):
+ return False
Review Comment:
this check overlooks the possibility where we want to compare with python's builtin `int`.
if we really want to support the case where we compare `PrimExpr` to `None`, the most conservative check should be:
```suggestion
if other is None:
return getattr(self, "handle", None) is None
```
however, i suppose that the TVM FFI should handle the case where IntImm is `IntImm(nullptr)`. Therefore, if it segfaults, it might be more fundamental and the fix I proposed shouldn't be the most proper. Would be great if we take a deeper dive
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] rafzi commented on pull request #12034: [TIR] fix crash when comparing IntImm to None
Posted by GitBox <gi...@apache.org>.
rafzi commented on PR #12034:
URL: https://github.com/apache/tvm/pull/12034#issuecomment-1178257749
@junrushao1994 Makes sense, thanks for elaborating!
I updated the PR according to your suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #12034: [TIR] fix crash when comparing IntImm to None
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on PR #12034:
URL: https://github.com/apache/tvm/pull/12034#issuecomment-1178227041
> https://github.com/apache/tvm/blob/013d5e8fcbd94fb3a0c5c0cdcaea03af43c464aa/src/tir/op/op.cc#L514
> https://github.com/apache/tvm/blob/013d5e8fcbd94fb3a0c5c0cdcaea03af43c464aa/src/tir/op/op.cc#L102
I see. In this case, the most proper fix should be adding nullability checks in this method:
```
CHECK(lhs.defined()) << "ValueError: `lhs` is null in the binary operator";
CHECK(rhs.defined()) << "ValueError: `rhs` is null in the binary operator";
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #12034: [TIR] fix crash when comparing IntImm to None
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on PR #12034:
URL: https://github.com/apache/tvm/pull/12034#issuecomment-1178252594
Thanks for your feedbacks!
To share some history: TVM’s older infra didn’t differentiate really carefully for nullability, which leads to the issue that this PR is aiming to fix. The latest infra in TVM instead use “Optional” container and “not-nullable” marks to make sure nullability is explicitly checked by engineers. However, older code isn’t refactored to completely adopt such convention.
In our particular case, empirically adding a check will not significantly impact performance, but I don’t have data either. If we really want to avoid throwing a segfault here, I would say its probably the best approach - other than completely refactoring the system to make PrimExpr not-nullable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org