tvm
tvm copied to clipboard
[Runtime][Bugfix] Added type-checking for Array::insert
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.
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.
cc @areusch
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.
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.
we probably should resolve https://discuss.tvm.apache.org/t/downgrade-tvm-runtime-to-c-11/13492/5 before merging this
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.
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.
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.