tqchen opened a new issue #5318: [REFACTOR][IR] Migrate ObjectRef to NotNULL and use Optional URL: https://github.com/apache/incubator-tvm/issues/5318 https://github.com/apache/incubator-tvm/pull/5314 Introduced NotNull and optional support to the IR data structure. We should start to track the migration of existing ObjectRef sub-classes to not-null by default to enhance the robustness throughout the codebase. ### Update Guide Right now the not-null ObjectRef is **opt-in**. That means by default an ObjectRef is nullable. We can gradually change the Ref types to not nullable, the steps are: - Change macro to `TVM_DEFINE_NOTNULLABLE_OBJECT_REF_METHODS`, - Remove the default constructor(if already defined) that corresponds to nullptr if there is a custom defined one. - If there is a method to construct an empty ObjectRef, we can use default constructor to do that(e.g. IRModule::Empty()->IRModule()). - Fix the compile error - For structs that takes the ref as a member, possibly due to unavailable default constructor, change to a new copy constructor - For cases that needs nullable version, use Optional See example change for String https://github.com/apache/incubator-tvm/pull/5314/files#diff-6597547f217e514b638f9548fda1dbcaR528 So hopefully the upgrade will be smooth and incremental instead of a one-time big change. In the meanwhile, we do recommend to directly start using `Optional<T>` in cases where nullptr behavior is required. Once we have migrated most of the Ref, we might decide to change the default behavior to **opt-out**. Which means, by default `_type_is_nullable` property for the ObjectRef is set to be true. ### Engineering Implications and Example Non-nullable refs does introduce a bit of engineering overhead. In particular, non-nullable refs may not have a default constructor(the default behavior of nullable refs defaults to nullptr) to let us enjoy more compile time checks. This means we need to introduce the member init constructor for each sub classes of ObjectNode to directly initialize them(to satisfy the invariance that these objects should be not null. Say PrimExpr is notnullable. ```c++ class RangeNode : public Object { public: PrimExpr min; PrimExpr extent; // because min/extent does not have default constructor // we have to explicitly create such constructors RangeNode(PrimExpr min, PrimExpr extent) : min(min), extent(extent) {} // rest of the code }; class Range : public ObjectRef { public: Range make_by_min_extent(PrimExpr min, PrimExpr extent) { // old-style no longer works, because there is no default constructor. // auto n = make_object<RangeNode>(); // n->min = std::move(min); // n->extent = std::move(extent); // return Range(n); // Need to directly call the constructor of RangeNode to intialize the fields. return Range(make_object<RangeNode>(min, extent)); } }; ``` ### Task Items The refactor should starts with leaf nodes, while keeping most of the generic nodes(Expr, Stmt) still nullable first, so that we don't have to change the constructor all at once. - [ ] Array, Map, ADT -- might affect topi and need to move some of things to Items - It will also affect the IR nodes that uses Array(e.g. CallNode), however, because Array has default constructor, it is fine to not change the IR nodes for now, although it will incurr additional cost(of constructing an Array then move another one by destroying the previous one). - [ ] IRModule - [ ] Pass/PassContext - [ ] Integer, Float, IntImm -- might affect topi - [ ] Leaf IR nodes of TIR nodes (e.g. AddNode, ForNode) - [ ] Leaf IR nodes of relay - [ ] Leaf nodes of type - [ ] BaseExpr, Type, TIR::Stmt
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
