[DISCUSS] Unify Relay::Value with Relay VM object

tqchen · January 6, 2020, 6:43pm

Previous, we use relay::Value to hold objects in the interpreter, and use relay::vm::Object to hold the objects in VM.

As we are moving toward a unified runtime object protocol, it would be really nice to unify these objects. So we no long have to say that “hey this object is specific for VM”, they are just normal tvm runtime objects.

Of course, we know that some of these objects need additional fields to hold reference to interpreter. These can be done via sub-classing (e.g. make TensorValue a sub-class of NDArray, perhaps we also want to bring a better name) so that from users’ perspective, they can treat them as runtime data structure.

Please share your thoughts and see if you are interested in help to push for this change.

tqchen · January 6, 2020, 6:44pm

cc @jroesch @MarisaKirisame @wweic @haichen @zhiics @frozengene

zhiics · January 6, 2020, 7:01pm

This probably would help us unify the objects used in VM and interpreter as well. We currently have separate Tensor objects even in VM and interpreter.

haichen · January 6, 2020, 7:22pm

I like the unification of Value and VMObject. Only thing we need to solve is how to handle closure value node.

tqchen · January 6, 2020, 7:53pm

would sub-classing suffice for the case? e.g. ClosureValue sub-class vm::closure, this of course depends on the common fields that they need to reuse

zhiics · January 6, 2020, 8:46pm

I can take a look. I remember they have quite different members. BTW, TensorValue and TensorObj should be safely replaced by NDArray, right?

wweic · January 7, 2020, 3:46am

Yes, closure requires some unification. The variable environment in interpreter is a compile time container Map, vm is std::vector. And function body in interpreter is the Function expression(with Stmt or PackedFunction, vm is a index to the compiled function(either sequence of bytecode or PackedFunction).

The environment maybe we can unify with runtime container. For function body I think they will have to diverge. Or we move the specific function body implementation into private area internal to interpreter or vm, in the closure we just store a reference(could be an integer).

github.com

apache/incubator-tvm/blob/a8c369218e87979020f732b9b2ad373fce4895f2/include/tvm/relay/interpreter.h#L93


    return static_cast<const ValueNode*>(get());
  }


  using ContainerType = ValueNode;
};


/*! \brief A Relay closure, i.e a scope and a function. */
class Closure;


/*! \brief The container type of Closures. */
class ClosureNode : public ValueNode {
 public:
  /*! \brief The set of free variables in the closure.
   *
   * These are the captured variables which are required for
   * evaluation when we call the closure.
   */
  tvm::Map<Var, Value> env;
  /*! \brief The function which implements the closure.
   *
   * \note May reference the variables contained in the env.

github.com

apache/incubator-tvm/blob/a1b6f4698110ef2916cebd2245a4ba5f001636c4/src/relay/backend/interpreter.cc

/*
 * Licensed to the Apache Software Foundation (ASF) under one
 * or more contributor license agreements.  See the NOTICE file
 * distributed with this work for additional information
 * regarding copyright ownership.  The ASF licenses this file
 * to you under the Apache License, Version 2.0 (the
 * "License"); you may not use this file except in compliance
 * with the License.  You may obtain a copy of the License at
 *
 *   http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */

/*!

This file has been truncated. show original

tqchen · January 7, 2020, 4:21am

In that case, perhaps we can start with a common base class that both vm and closureValue can sub-class.

zhiics · January 7, 2020, 4:31am

Should we directly remove TensorObj and TensorValue, and use NDArray directly? I spent some time on it this afternoon. If so, I can finish that first.