[RFC] Node Construction Style: Use Constructor When Possible

[tqchen]

Node system lays the foundation to build TVM's AST structures. Each AST object corresponds to a subclass of Node. Besides the Node itself, we also usually defines a reference type which can be used to store a strong reference to the node. The following code example shows how to define Node XYZNode and reference type XYZ.

class XYZ;

class XYZNode : public Node {
 public:
   static XYZ make(args);
};

class XYZ : public NodeRef {
  public:
   ...
};

void MyFunction() {
   XYZ ref = XYZNode::make(args);
}

Historically, we introduced a static factory function make to each Node type which creates a new reference to the Node(as shown in the above code). This is less natural compared to direct usage of a constructor or moves the static factory function into the reference class. As shown in below.

class XYZNode : public Node {
 public:
};

class XYZ : public NodeRef {
  public:
   XYZ(args);
   ...
};

void MyFunction() {
   // use constructor.
   XYZ ref = XYZ(args);
}

At the moment, not all the Node have a specific reference type -- this is also the reason why we adopt make in the Node historically. However, we start to see a trend of giving a specific reference type to each of the new Node introduced(e.g. in relay). I would also like to mention that we have already transitioned the python API to directly use the constructor-style code. Favor constructors in c++ will make both sides of the code to be more consistent.

In very rare cases, using constructors could cause ambiguity. In particular, the expression Not(Not(a)) could be seen as constructing two-levels of Not expressions or trying to copy construct Node. In such cases, perhaps we still want to use a static factory function inside the reference.

This RFC tries to see if we want to migrate to the new style, specifically:

• For Nodes that do not have a corresponding reference type, keep the make function on Node.
• For Nodes that have a reference type
  • Favor constructor over XYZNode::make
  • Favor static factory functions in the reference type.

Most of the changes will be quite mechanical and we can do it incrementally. Please share your thoughts.

[tqchen]

I also created #3224 as an example of changes to be made to fit the new style.

[junrushao]

I was wondering what are the cons to have a static factory function

[tqchen]

Static factory function is fine. The only question is whether they should be in the Node class or the reference class itself.

[tqchen]

Some further thoughts: in many cases, a static factory function gives more information than a constructor. For example, NDArray::Empty is certainly better than just using NDArray's constructor.

So we do want to keep static factory function if they are in reference types. Perhaps the usage of the constructor should be restricted to the case where it is apparently initializing the corresponding fields in the node, which is the case raised in the PR.

However, even if we want to use static factory functions, they should be in the NodeRef class, as opposed to the Node class.

[tqchen]

Thanks for all the discussions, a summary of guidelines:

• For constructor that initializes the fields, use the constructor.
• Encourage static factory function on the reference data type(instead of the Node) if there are more complicated construction logic and the function naming gives more clear information.