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

[REFACTOR][IR] Migrate ObjectRef to not-null and use Optional #5318

Closed
1 of 8 tasks
tqchen opened this issue Apr 13, 2020 · 2 comments
Closed
1 of 8 tasks

[REFACTOR][IR] Migrate ObjectRef to not-null and use Optional #5318

tqchen opened this issue Apr 13, 2020 · 2 comments

Comments

@tqchen
Copy link
Member

tqchen commented Apr 13, 2020

#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.
    • Rationale: by removing the default constructor, it is unlikely to see an not-nullable ref to hold a nullptr, except for the case when it is moved(in which case we should not refer to then anyway)
    • 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.

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. Most of the tasks can be done in parallel, and only involves mechanical changes.

  • Array, Map, ADT -- might affect topi and need to move some of things to Items
    • NOTE: change to array 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 (depends on previous leaf node tasks)
@tqchen tqchen changed the title [REFACTOR][IR] Migrate ObjectRef to NotNULL and use Optional [REFACTOR][IR] Migrate ObjectRef to not-null and use Optional Apr 13, 2020
@tqchen
Copy link
Member Author

tqchen commented Apr 13, 2020

@tqchen
Copy link
Member Author

tqchen commented Jun 14, 2020

NOTE: The notnull support for composite types has a bit more complications, we will stop working on it for now and then revisit once w ehave a solution.

@areusch areusch added the needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it label Oct 19, 2022
@areusch areusch added core:object and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Nov 16, 2022
@tqchen tqchen closed this as completed Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants