Skip to content

[High level OPT][RFC] NNVMv2 IR - Relay - #1672

Merged
tqchen merged 136 commits into
apache:masterfrom
jroesch:relay
Sep 19, 2018
Merged

tqchen merged 136 commits into
apache:masterfrom
jroesch:relay

Conversation

@jroesch

@jroesch jroesch commented Aug 29, 2018 •

Copy link
Copy Markdown
Member

[RFC]: Relay a new high level IR for TVM

Relay is a new high level intermediate representation (IR) intended to act as v2.0 of NNVM.

See #1673 for the full RFC details.

Python 2.7 Support

Our goal is to adapt the core IR code to support 2.7 as well as 3.X. We plan on addressing this near the end of the PR review window after other TODOs have been addressed.

Features

This PR was extracted from a larger work-in-progress version of Relay, and as such it still needs some additional features before it is ready to merge into TVM mainline. We are opening the PR in a "nearly finished" state so other contributors can provide review and feedback on the alpha design and implementation of Relay.

Core IR

  • Add base nodes for IR
  • Add Type nodes for IR
  • Add Expr nodes for IR
  • IR C++ Docs
  • IR Python Docs
  • Add operator registry with TVM integration
  • Add new attribute system exposed in C++ and Python
  • Improve construction of nodes
  • Add type inference for Relay
    • Fix helpers in the Environment
    • Add few more type relation examples.
    • Restore kind checker
    • Increase test coverage
  • Add compiler to TVM runtime for Relay
    • Port mono-morphization pass
    • Port TVM RTS compiler
  • Demonstrate writing a pass in both C++ (type inference) and Python.
  • IR Builder Docs
  • Restore code for printing NDArray's in Numpy style
  • Restore error display

Code Map

This section contains a high level overview of the code organization by file.

Core IR

  • include/tvm/relay/base.h and src/relay/ir/base.cc contain the base classes for Relay AST.
    We reuse the tvm::Node infrastructure to enable Python interoperability.
  • include/tvm/relay/type.h and src/relay/ir/type.cc contain the type classes for the Relay Type AST.
  • include/tvm/relay/expr.h and src/relay/ir/expr.cc contain the expression classes for the Relay expression AST. This is a small functional language with functions, control-flow, operators, and constants.
  • include/tvm/relay/op.h and src/relay/ir/op.cc contains the definition of the Relay operator registry. Relay's operator registry stores high level information about operators such as type, and metadata useful for optimization similar to NNVM.
  • include/tvm/relay/environment.h and src/relay/ir/environment.cc contains the global environment, which contains all global functions and configuration options for the compiler.
  • include/tvm/relay/error.h
  • include/tvm/relay/expr_functor.h and include/tvm/relay/expr_visitor.h define traversal strategies over Relay expressions.
  • include/tvm/relay/logging.h contains a wrapper around logging which can be controlled with an environment variable. This is useful for extra logging information while debugging the compiler.
  • include/tvm/relay/pass.h exports the current set of Relay passes (only type inference right now).
  • include/tvm/relay/source_map.h src/relay/source_map.cc

Operators

  • src/relay/op/tensor/elemwise.cc provides examples of registering Relay operators.
  • src/relay/op/type_relations.cc and src/relay/op/type_relations.h defines common
    type relations (i.e shape inference functions) for operators.

Type Inference

  • include/tvm/relay/pass/type_infer.h and src/relay/pass/type_infer.cc implement type inference, and type checking. Given a Relay expression (e) they infer a type (T) written (e : T) and check that e has type T. This process proceeds by building partial types for an expression and iteratively filling the type in by solving constraints introduced in the program.
  • src/relay/pass/incomplete_type.h introduces an "incomplete type" (known as a type variable in the programming language and compiler literature), this represents a portion of the type we do not yet know.
  • src/relay/pass/unifier.h and src/relay/pass/unifier.cc implement type unification. They solve equations between types which arise due to type inference/checking, the idea is that there are some unknown portions of the type (incomplete types) and our goal is to fill them in given a set of equations on types.
  • include/tvm/relay/pass/alpha_eq.h and src/relay/pass/alpha_eq.cc implements structural equivalence of Relay types and expressions. This is called alpha equivalence in the programming language and compiler literature.
  • src/relay/pass/resolve.h and src/relay/pass/resolve.cc implement a procedure which will resolve incomplete types into complete ones based on solutions in the unifier.
  • src/relay/pass/type_functor.h and src/relay/pass/type_visitor.h implement useful traversal strategies over types.
  • src/relay/pass/type_subst.h and src/relay/pass/type_subst.cc implement type substitution.

Python Core IR

  • python/tvm/relay/__init__.py is the entry point of the Relay namespace.
  • python/tvm/relay/_env.py exports the FFI to the Environment and python/tvm/relay/_env.pyi adds MyPy stubs.
  • python/tvm/relay/_ir_pass.py exports the FFI to the pass functions and python/tvm/relay/_ir_pass.pyi adds MyPy stubs.
  • python/tvm/relay/_make.py exports the constructor functions used in expr.py, etc.
  • python/tvm/relay/base.py, python/tvm/relay/env.py, python/tvm/relay/expr.py, and python/tvm/relay/type.py export their C++ counterparts to Python.
  • python/tvm/relay/ir_builder.py contains a first attempt at an IRBuilder for Relay.
  • python/tvm/relay/ir_pass.py contain the pass functions exported by Relay.

Python Operators

  • python/tvm/relay/op/__init__.py entry point into relay.op namespace.
  • python/tvm/relay/op/_make.py exposes functions which build calls to each op.
  • python/tvm/relay/op/tensor.py contains explicit definitions of operators which dispatch to the primitives defined in op/_make.py, this means Relay's Python API has explicit signatures for each op, and don't rely on *args and **kwargs.
  • python/tvm/relay/op/_tensor.py backend registry for compiler related data about ops in tensor.py.
  • python/tvm/relay/op/op.py exposes functionality on the Operator Registry.

Python Compiler to TVM RTS

  • python/tvm/relay/tvm_rts_backend.py contains a soon to be updated version of a compiler from Relay to the TVM RTS.

Timeline

  • Complete PR checklists.
  • Leave PR open for ~2 weeks for review and feedback while finishing up TODOs.
  • Merge PR with @tqchen when he returns from vacation.

Thanks

Finally I would like to thank all the people who helped out with the development of Relay. Although I am the author of most of these commits the work is derived from months of hard work from @MarisaKirisame @slyubomirsky @joshpoll @weberlo @tqchen and @ztatlock.

@junrushao

junrushao commented Aug 29, 2018 •

Copy link
Copy Markdown
Member

Nice work! I believe that this is the first elegant and systematic approach in addressing the lack of Turing-completeness in deep learning systems. Will be happy to see the future development of Relay to support richer sets of features that could possibly benefit deep learning practitioners. Will roughly go through it in 24 hours and leave more comments.

BTW, @jroesch could you add a link or bibtex to the Relay paper in the description so that others could easily cite?

@junrushao

junrushao commented Aug 29, 2018 •

Copy link
Copy Markdown
Member

This is pretty cool feature to make the dependent type shape of an NDArray generic, but is this able to be dynamic? More specifically, the shape depends on some intermediate result of computation?

Update: my concern is resolved after reading the code.

@masahi

masahi commented Aug 29, 2018

Copy link
Copy Markdown
Member

@jroesch how does it relate to HalideIR?

@tqchen tqchen changed the title Add initial version of the Relay IR [High level OPT][RFC] NNVMv2 IR Aug 29, 2018
@tqchen tqchen added the status: need RFC need RFC discussion label Aug 29, 2018
@tqchen

tqchen commented Aug 29, 2018

Copy link
Copy Markdown
Member

Let us move general discussion to a RFC thread, and keep this thread for specific code discussion

@tqchen tqchen changed the title [High level OPT][RFC] NNVMv2 IR [High level OPT][RFC] NNVMv2 IR - Relay Aug 29, 2018
@jroesch

jroesch commented Aug 29, 2018

Copy link
Copy Markdown
Member Author

@masahi just moved the proposal bit to the RFC (see #1673) let's chat there.

Comment thread include/tvm/relay/environment.h Outdated
* It contains all global functions, and configuration
* options.
*
* Many operations require acess to the global

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"acess" => "access"

Comment thread include/tvm/relay/environment.h Outdated
#include <string>
#include <vector>
#include "./expr.h"
#include "./type.h"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably should prefer absolute path and configure include path...

Comment thread src/relay/source_map.cc Outdated
}
}

std::string SourceFragment::SourceAt(Span sp, int max_lines) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to default max_lines to 0, and return the current line in that case.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about defaulting to 1? unless I'm missing something?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I probably misunderstood it. 1 works if it means the current line.

Comment thread src/relay/source_map.cc Outdated
return source_slice;
}

SourceName SourceMap::AddSource(std::string file_name, std::string source) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, do it make sense to allow source replacement, e.g. replace a line or a chunk of code?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, the current design is to enable three use cases.

  1. The user uses a Relay frontend (such as the one we prototyped) and we register the chunk of Python code written by the user.

  2. We parse the IR from a textual format and are able to report directly against the file inputted.

  3. We generate a source representation of a programmatic IR which we use to report errors against. The idea is the user builds a graph by hand either in Python or one of the frontends, we pretty print and then parse the result giving us a "source program" to show the error against.
    This final one is very useful as we can trigger errors when trying to optimize and point directly to a cleanly printed representation of the program.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks.

Comment thread include/tvm/relay/environment.h Outdated
Function Lookup(const std::string & s);

/*! \brief Add a source fragment to the environment. */
SourceName AddSource(std::string file_name, std::string source);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: make both of them const std::string& ?

Comment thread include/tvm/relay/type.h
Comment thread src/relay/pass/unifier.h Outdated
explicit UnionFind(std::shared_ptr<tvm::Node> p) : NodeRef(p) {}

// no const so that union find can be mutable as a member of unifier
inline UnionFindNode* operator->() const {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no need to put inline before member functions, compiler will automatically inline them if it is necessary/profitable.

Comment thread include/tvm/relay/expr.h Outdated

RELAY_DEFINE_NODE_REF(Function, FunctionNode, Expr);

using Attrs = tvm::Attrs;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Move it into class CallNode? Seems that's the only place you need it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this is an old artifact of when this was typedef'd to something else. I will just remove and use tvm::Attrs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also prefer tvm::Attrs

Comment thread include/tvm/relay/expr_functor.h
Comment thread include/tvm/relay/expr_visitor.h Outdated
for (auto param : op->params) {
Expr param_expr = this->VisitExpr(param, args...);
if (const ParamNode* param_node = param_expr.as<ParamNode>()) {
auto param = GetRef<Param>(param_node);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change variable name? it has the same name as the auto param

@masahi

masahi commented Aug 30, 2018

Copy link
Copy Markdown
Member

Another style issue: it looks like & position in function arguments are basically random everywhere. They need to be consistent.

Comment thread include/tvm/relay/pass/type_infer.h Outdated
/*! \brief Ensures that an operator is well-formed with respect
* to Relay's type system.
*/
Op CheckOp(const Environment & env, const Op & op);

@masahi masahi Aug 30, 2018 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a more descriptive name would be helpful.

Comment thread include/tvm/relay/base.h Outdated
/*!
* \brief we always used NodeRef for referencing nodes.
*
* By default, NodePtr is a std::shared_ptr of node

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment intended to say NodeRef rather than NodePtr ?

Comment thread include/tvm/relay/pass/type_infer.h Outdated
* The pass produces a new expression with its checked_type
* field populated and incomplete types resolved.
*/
#ifndef TVM_RELAY_PASS_TYPECHECKER_H_

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TYPE_INFER_H_

Comment thread include/tvm/relay/source_map.h Outdated
namespace tvm {
namespace relay {

struct SourceFragment {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need explaination of the example usage.

Comment thread include/tvm/relay/source_map.h Outdated
struct SourceFragment {
std::string file_name;
std::vector<std::string> source_lines;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider make it as a Node if we are supposed to access from python

Comment thread include/tvm/relay/expr_visitor.h Outdated
};

template <typename... Args>
class ExprFVisitor : public ::tvm::relay::ExprFunctor<Expr(const Expr& n, Args...)> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make things consistent with low-level naming convention, this should be ExprMutator. See https://github.com/dmlc/tvm/blob/master/include/tvm/ir_mutator.h

@tqchen tqchen Aug 31, 2018 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider make function signature as the follows(which is consistent with tvm::IRMutator)

Expr Mutate_(const ConstOpNode* op, const Expr& self) {
}

This removes the need of GetRef in the default leaf case

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I would prefer to never use GetRef but it doesn't provide us the down-casted data which would be far more useful. Is it the extra argument useful enough to be passed everywhere?

Comment thread include/tvm/relay/expr_visitor.h Outdated

void VisitExpr_(const TupleNode* op, Args... args) override {
for (auto field : op->fields) {
this->VisitExpr(field, args...);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to use std::forward(args)...

Comment thread include/tvm/relay/logging.h
Comment thread include/tvm/relay/pass.h Outdated

/*! The result of type checking an expression is a new expression
* with unambigous type information filled in, as well as it's
* checked type field populated with the result type.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document all parameters and return values

Comment thread include/tvm/relay/pass/alpha_eq.h Outdated
#include "../expr.h"

namespace tvm {
namespace relay {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider directly move the relay/pass.h for now

Comment thread include/tvm/relay/pass/alpha_eq.h Outdated
namespace tvm {
namespace relay {

bool AlphaEqual(const Expr & e1, const Expr & e2);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: const Expr&

Comment thread include/tvm/relay/pass/type_infer.h Outdated

/*! \brief Ensures that an operator is well-formed with respect
* to Relay's type system.
*/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to relay/pass.h

Comment thread include/tvm/relay/expr_visitor.h Outdated
};

template <typename... Args>
class ExprFVisitor : public ::tvm::relay::ExprFunctor<Expr(const Expr& n, Args...)> {

@tqchen tqchen Aug 31, 2018 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider make function signature as the follows(which is consistent with tvm::IRMutator)

Expr Mutate_(const ConstOpNode* op, const Expr& self) {
}

This removes the need of GetRef in the default leaf case

@joshpoll joshpoll left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly comments on comments. Will leave more soon.

Comment thread include/tvm/relay/base.h Outdated
/*!
* \brief Macro to make it easy to define node ref type given node
* \param TypeName The name of the reference type.
* \param NodeName The internal contrainer name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contrainer -> container

Comment thread include/tvm/relay/base.h Outdated
class Span;
/*!
* \brief Stores locations in frontend source that generated a node.
*

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra line

Comment thread include/tvm/relay/base.h
Comment thread include/tvm/relay/base.h Outdated
*/
class SourceName;
/*!
* \brief The source name in the Span

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an identifier or source code? The comment should make this clear.

Comment thread include/tvm/relay/base.h Outdated
*/
class RelayNode : public Node {
public:
/*! \brief The debug information, can be null, check with span.defined() */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't exactly true. It's just source map info. What about: "Location of node in source." or something.

Comment thread include/tvm/relay/expr.h
Comment thread include/tvm/relay/expr.h Outdated
/*! \return The corresponding tensor type of the data */
TensorType tensor_type() const;

/*! \return whether it is scalar(rank-0 tensor) */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether it is scalar (i.e., a rank-0 tensor).

Comment thread include/tvm/relay/expr.h Outdated
Comment thread include/tvm/relay/expr.h Outdated
* \brief Local variables used in the let expression.
* This is similar to Var that is being used in the low level tensor expression.
*
* \note Each LocalVar is bind only once and is immutable/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are bind-once and immutability different? Also the comment should end in a period.

Comment thread include/tvm/relay/expr.h Outdated
/*! \brief Container for LocalVar */
class LocalVarNode : public ExprNode {
public:
/*! \brief The name of the variable, this only acts as a hint. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> The name of the variable. This only acts as a hint.

What does hint mean? Who does it provide a hint for?

Comment thread include/tvm/relay/environment.h Outdated
void Update(const GlobalVar& var, const Function & func);
void Remove(const GlobalVar& var);

/*! \brief Lookup a global function by its variable. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*name ?

Comment thread include/tvm/relay/expr.h Outdated
/*! \brief The expression evaluated when condition is true. */
Expr true_value;
/*! \brief The expression evaluated when condition is false */
Expr false_value;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be called true_expr and false_expr or maybe true_branch and false_branch?

Comment thread include/tvm/relay/logging.h
Comment thread include/tvm/relay/op.h Outdated
}
return *this;
}
/*! \return The global single retistry */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*registry

Comment thread include/tvm/relay/op.h
Comment thread src/relay/ir/op.cc Outdated
std::unordered_map<std::string, std::unique_ptr<GenericOpMap> > attr;
// frontend functions
std::vector<PackedFunc*> frontend_funcs;
// get singleton of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete comment?

Comment thread src/relay/pass/alpha_eq.cc
Comment thread src/relay/pass/unifier.cc Outdated
}

void UnionFindNode::unify(const IncompleteType &v1, const Type &t) {
RELAY_LOG(INFO) << "UnionFindNode::Unify v1=" << v1 << "t=" << t << std::endl;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should "t=" be " t="?

Comment thread src/relay/pass/unifier.h Outdated
Comment thread src/relay/source_map.cc Outdated
std::stringstream out;

// We need to move from 1 based indexing to zero based indexing.
int starting_line = sp->lineno;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this comment have to do with the line that follows?

Comment thread include/tvm/relay/error.h Outdated
namespace relay {

struct Error : dmlc::Error {
Error(std::string msg) : dmlc::Error(msg) {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2-space indent instead. and shall we use explicit?

Comment thread include/tvm/relay/error.h Outdated
};

struct InternalError : Error {
InternalError(std::string msg) : Error(msg) {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

Comment thread include/tvm/relay/expr_visitor.h Outdated
auto type = this->VisitType(op->type);
return ParamNode::make(var, type);
} else {
throw dmlc::Error("the default param visitor has bug");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is better to use relay::Error instead.

Comment thread include/tvm/relay/expr_visitor.h Outdated
auto ty_param_ref = GetRef<TypeParam>(ty_param);
ty_params.push_back(ty_param_ref);
} else {
throw dmlc::Error("the default func visitor has bug");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relay::Error

Comment thread include/tvm/relay/error.h Outdated
namespace relay {

struct Error : dmlc::Error {
Error(std::string msg) : dmlc::Error(msg) {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const std::string&

Comment thread include/tvm/relay/error.h Outdated
};

struct InternalError : Error {
InternalError(std::string msg) : Error(msg) {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Comment thread include/tvm/relay/error.h Outdated
struct SpannedError {
std::string msg;
Span sp;
SpannedError(std::string msg, Span sp) : msg(msg), sp(sp) {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Comment thread include/tvm/relay/expr.h Outdated
* \brief Global variable that leaves in the top-level environment.
* This is used to enable recursive calls between function.
*
* \note GlobalVar can only corresponds to functions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/corresponds/correspond

Comment thread include/tvm/relay/expr.h
Comment thread include/tvm/relay/expr.h
Comment thread include/tvm/relay/expr_functor.h Outdated
* \sa tvm/ir_functor.h
*
* \tparam FType function signiture
* This type if only defined for FType with function signiture R(const Expr&,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/if/is

Comment thread include/tvm/relay/type.h Outdated
kType = 3,
};
/*!
* \brief The variable

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/The variable/d

Comment thread src/relay/pass/unifier.cc Outdated

void UnionFindNode::debug() {
for (auto entry : this->uf_map) {
std::cout << entry.first << " = " << entry.second << std::endl;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use LOG(INFO) instead of std::cout?

Comment thread python/tvm/relay/type.py Outdated
ShapeVar = 0
Shape = 1
BaseType = 1
Type = 2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enum Kind : int {
/*! \brief template variable in shape expression */
kShapeVar = 0,
kShape = 1,
kBaseType = 2,
kType = 3,
};
I am certain that this is an error

Comment thread src/relay/ir/op.cc Outdated
std::unordered_map<std::string, std::unique_ptr<GenericOpMap>> attr;
// frontend functions
std::vector<PackedFunc*> frontend_funcs;
// get singleton of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment is off

Comment thread src/relay/ir/op.cc
// get singleton of the
static OpManager* Global() {
static OpManager inst;
return &inst;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not return OpManager&?

Comment thread src/relay/ir/op.cc
Comment thread src/relay/ir/op.cc Outdated
if (auto ty_func = op.as<FuncTypeNode>()) {
return ty_func->type_params.size() != 0;
} else {
return false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be an error instead?

return imm->value;
}

Array<Type> IdentityRel(const Array<Type>& types, int num_args) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it need num_args when we can always get it from types?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the array contains input and output variables, we made it this way to simplify the calling convention between C++ and Python.

Comment thread src/relay/op/type_relations.cc Outdated
<< (full_len - suffix_len - 1) << std::endl;
auto lower_bound = full_len - suffix_len - 1;

for (int64_t i = full_len - 1; i > lower_bound; i--) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will out of range immediately on lb of different size

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suppose sh1.size() is 10, sh2.size() is 5
lb = 4
i originally = 9
will out of bound

Comment thread src/relay/op/type_relations.cc Outdated
std::cout << "Index i=" << i << std::endl;
auto dim1 = to_int(sh1[i]);
auto dim2 = to_int(sh2[i]);
if (dim1 != dim2) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one of them can be 1 in this case, no need to immediately Check false

@jroesch

jroesch commented Sep 6, 2018

Copy link
Copy Markdown
Member Author

Thanks for feedback going to try and get PR in a clean state tomorrow. I spent a lot of today adding the final features we need and looking into updating docs.

Comment thread python/tvm/relay/tvm_rts_backend.py Outdated
def lookup(self, ident: ir.LocalId) -> NodeRef:
return self.id_map[ident]

def compile(self, func: ir.Function) -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantages of keeping compile algorithm implemented in Python rather than in C++? I would say that keep it in C++ is better because it allows us to write DSL frontends for other languages more easily.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually we will port it to C++, the idea was to demonstrate some passes in C++ and Python, as the alpha of Relay progresses more and more will be moved into C++.

Comment thread include/tvm/relay/expr.h
/*!
* \brief Stores the result of type inference(type checking).
*
* \note This can be undefined before type inference.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this note as it is now serialized

@jroesch jroesch Sep 19, 2018 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is still accurate, before type inference the node may still contain null.

Comment thread include/tvm/relay/expr.h
Comment thread src/relay/op/type_relations.cc Outdated
rev_sh2++;
}

Array<HalideIR::Expr> larger;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ShapeExpr

Array<HalideIR::Expr> smaller;

for (int i = 0; i < (full_len - suffix_len); i++) {
smaller.push_back(tvm::ir::IntImm::make(HalideIR::Int(64), 1));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Int function is already defined

@jroesch

jroesch commented Sep 19, 2018

Copy link
Copy Markdown
Member Author

I think this is now in a state where we should merge and address any other issues in follow-up PRs.

@tqchen
tqchen merged commit 51fe00f into apache:master Sep 19, 2018
@tqchen

tqchen commented Sep 19, 2018

Copy link
Copy Markdown
Member

Thanks, @yzhliu @jroesch @alex-weaver @zhiics @junrushao1994 @masahi @joshpoll @weberlo @grwlf @MarisaKirisame for reviews.

I merge this PR given that it reaches a state of test passing and most comments addressed. Let us get the balls rolling with follow up PRs

@jroesch

jroesch commented Sep 19, 2018

Copy link
Copy Markdown
Member Author

I plan on opening two PRs one for the evaluation and one for the documentation and reference manual.

FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018
@jroesch
jroesch deleted the relay branch February 4, 2021 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.