[Discuss][UX] `IRFunctor` vs `ExprFunctor` and other naming issues

[slyubomirsky]

As discussed in the community meeting, ExprMutator and ExprVisitor somewhat confusingly also traverse binding blocks, which are statements rather than expressions. What is even more interesting is that these inherit from ExprFunctor, which does not traverse binding blocks. By contrast, IRFunctor (which is only used in one place in the codebase) does traverse binding blocks, but is not the parent class of ExprMutator and ExprVisitor. The naming here is very confusing and should probably be straightened out.

There are a few routes we could take (these are not exhaustive):

1. Eliminate ExprFunctor and rename ExprMutator and ExprVisitor to IRMutator and IRVisitor.
2. Rename the current ExprMutator and ExprVisitor to IRMutator and IRVisitor but maintain the distinction.

The main question is whether we have a need for functors that operate only on exprs and not on bindings. Option 1 seems like it probably wouldn't cause problems in the current state of the codebase, but who knows what the future holds. If we go with option 2, should we have a visitor just for binding blocks (a StmtVisitor, say)?

[YuchenJin]

Thanks @slyubomirsky for pointing this out!

I agree that we don't have use cases where we only want to traverse Expr but not traverse binding blocks and bindings inside, so I think Option 1 is a positive move: remove ExprFunctor, and make the current ExprMutator and ExprVisitor subclass IRFunctor.

One thing to consider is that the current ExprMutator::VisitBinding does not take a binding and returns a binding, instead has no return and emit binding inside. This is because when visiting bindings we may want to add bindings or remove binding. If we are going to make ExprMutator to subclass IRFunctor, binding visitor(VisitNode_(VarBindingNode*)) should be an interface that users can override, and instead we expose a ReEmitBinding interface.

Also, @ganler suggests we rename the current ExprMutator to ANFMutator to make it explicit to pass writers that this mutator operates on an ANF IR.

[slyubomirsky]

I very much like the idea of including "ANF" in the name of the mutator. The issue with visiting binding nodes is an interesting one. I don't think it necessarily causes a problem for users because they can override the visit to the binding blocks and that will give them the flexibility to emit or not emit a binding as needed.

[LeshengJin]

We might miss ExprMutatorBase in expr_functor

relax/include/tvm/relax/expr_functor.h

Lines 202 to 210 in 25dcbcb

	/*!
	* \brief A mutator works in unnormalized form.
	*
	* ExprMutatorBase expects input AST to be in the unnormalized form, i.e., checked_type_ and shape_
	* of expressions can be nullptr, and the expressions may nest(and as a result the AST is not in
	* ANF).
	*/
	class ExprMutatorBase : public ExprFunctor<Expr(const Expr&)> {

ExprMutatorBase can work on unnormalized IR. So maybe we can consider

Solution: rename ExprVisitor -> IRVisitor, ExprMutatorBase -> IRMutator, ExprMutator -> ANFMutator.

But ExprMutatorBase doesn't have functions, like VisitBinding or VisitVarDef. We may need to add them.