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/09/02 18:04:30 UTC
[GitHub] [tvm] Lunderberg opened a new pull request, #12691: [Runtime][Bugfix] Added type-checking for Array::insert
Lunderberg opened a new pull request, #12691:
URL: https://github.com/apache/tvm/pull/12691
Prior to this commit, the following code would compile and run without error. This occurs because the typed `Array<T>::insert` calls the untyped `ArrayNode::InitRange`, with no type-checking done before the call.
```c++
Var x("x");
Var y("y");
Array<Var> var_arr{x, y};
Array<PrimExpr> expr_arr{x + 1, y + 2};
// Erroneously inserts static-type PrimExpr, runtime-type Add, even
// though neither PrimExpr is a type of Var.
var_arr.insert(var_arr.begin(), expr_arr.begin(), expr_arr.end());
```
After this commit, a `static_assert` in `Array<T>::insert` and in `Array<T>::Array(IterType,IterTYpe)` restricts the iterators, such that they must dereference to `T`, `Optional<T>`, a subclass of `T`, or `Optional<U>` where `U` is a subclass of `T`.
The public method `ArrayNode::SetItem` exposes a similar issue. In the future, we may want to make it be private, accessed only through type-safe method in `Array<T>::Set`.
--
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] areusch commented on a diff in pull request #12691: [Runtime][Bugfix] Added type-checking for Array::insert
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #12691:
URL: https://github.com/apache/tvm/pull/12691#discussion_r981473953
##########
src/te/schedule/schedule_lang.cc:
##########
@@ -200,7 +200,7 @@ Stage& Stage::env_threads(Array<IterVar> threads) {
ICHECK_EQ(self->env_threads.size(), 0U) << "Already set env_threads";
Array<IterVar>& leaf_vars = self->leaf_iter_vars;
Array<IterVar>& all_vars = self->all_iter_vars;
- std::vector<ObjectRef> temp;
+ std::vector<IterVar> temp;
Review Comment:
oh i see, the diff hid line 207 by default, but now i see that temp gets placed into leaf_vars. that makes much more sense :)
--
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] Lunderberg commented on pull request #12691: [Runtime][Bugfix] Added type-checking for Array::insert
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #12691:
URL: https://github.com/apache/tvm/pull/12691#issuecomment-1238135540
Breakage is in the iOS build, as the clang flags were still using `-std=gnu++14`. Fixed in https://github.com/apache/tvm/pull/12712, will re-run CI after it lands.
--
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] Lunderberg commented on pull request #12691: [Runtime][Bugfix] Added type-checking for Array::insert
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #12691:
URL: https://github.com/apache/tvm/pull/12691#issuecomment-1251147656
From the discussion, it sounds like C++17 shouldn't be a blocker anymore, since there are no specific use cases at the moment, and multiple options to resolve it if/when they occur.
--
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] Lunderberg commented on pull request #12691: [Runtime][Bugfix] Added type-checking for Array::insert
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #12691:
URL: https://github.com/apache/tvm/pull/12691#issuecomment-1235845665
Breakage is in the WASM build, as the EMCC flags were still using `-std=c++14`. Fixed in https://github.com/apache/tvm/pull/12693, will re-run CI after it lands.
--
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] areusch commented on pull request #12691: [Runtime][Bugfix] Added type-checking for Array::insert
Posted by GitBox <gi...@apache.org>.
areusch commented on PR #12691:
URL: https://github.com/apache/tvm/pull/12691#issuecomment-1239838627
we probably should resolve https://discuss.tvm.apache.org/t/downgrade-tvm-runtime-to-c-11/13492/5 before merging this
--
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] Lunderberg commented on a diff in pull request #12691: [Runtime][Bugfix] Added type-checking for Array::insert
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #12691:
URL: https://github.com/apache/tvm/pull/12691#discussion_r979046683
##########
src/te/schedule/schedule_lang.cc:
##########
@@ -200,7 +200,7 @@ Stage& Stage::env_threads(Array<IterVar> threads) {
ICHECK_EQ(self->env_threads.size(), 0U) << "Already set env_threads";
Array<IterVar>& leaf_vars = self->leaf_iter_vars;
Array<IterVar>& all_vars = self->all_iter_vars;
- std::vector<ObjectRef> temp;
+ std::vector<IterVar> temp;
Review Comment:
This was actually the exact type of error that I wanted to catch with the `static_assert` statements. This was a `std::vector<ObjectRef>` which was copied into an `Array<IterVar>`, with no type-checking during the conversion. It didn't cause any runtime issues, because the `std::vector` was originally populated with `ObjectRef` instances that contain `IterVarNode`, but there were no compile-time checks to ensure that was the case. Had it been otherwise, the later use of `ObjectRef::DowncastNoCheck<IterVar>` would have invoked undefined behavior.
--
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] Lunderberg commented on pull request #12691: [Runtime][Bugfix] Added type-checking for Array::insert
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #12691:
URL: https://github.com/apache/tvm/pull/12691#issuecomment-1240983513
Sounds reasonable, and I've added a comment weighing in on that thread. If it becomes a sticking point on this PR, I can rewrite to avoid C++17, but C++17 has enough benefits that it would be disappointing to revert entirely.
--
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] areusch merged pull request #12691: [Runtime][Bugfix] Added type-checking for Array::insert
Posted by GitBox <gi...@apache.org>.
areusch merged PR #12691:
URL: https://github.com/apache/tvm/pull/12691
--
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] Lunderberg commented on pull request #12691: [Runtime][Bugfix] Added type-checking for Array::insert
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #12691:
URL: https://github.com/apache/tvm/pull/12691#issuecomment-1255253699
Rebased onto main, as https://github.com/apache/tvm/pull/12692 also needed and implemented the same type-checking structs, which makes this PR much smaller.
--
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] areusch commented on a diff in pull request #12691: [Runtime][Bugfix] Added type-checking for Array::insert
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #12691:
URL: https://github.com/apache/tvm/pull/12691#discussion_r977856513
##########
src/te/schedule/schedule_lang.cc:
##########
@@ -200,7 +200,7 @@ Stage& Stage::env_threads(Array<IterVar> threads) {
ICHECK_EQ(self->env_threads.size(), 0U) << "Already set env_threads";
Array<IterVar>& leaf_vars = self->leaf_iter_vars;
Array<IterVar>& all_vars = self->all_iter_vars;
- std::vector<ObjectRef> temp;
+ std::vector<IterVar> temp;
Review Comment:
this change seems unrelated?
--
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