You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/07/29 07:43:27 UTC
[GitHub] [incubator-tvm] jroesch opened a new pull request #6162: [Parser] Parser 2.0 part 2
jroesch opened a new pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162
This PR implements both metadata parsing and converts everything to the new diagnostic machinery.
I initially believed that the right way to parse the metadata table is to generate delayed references to the metadata table and then expand them after parsing is complete. In order to do this we need a piece of syntax i.e Expr, Type, Attrs, and so on for each meta reference. Currently this only works for `Expr` as I tried to define MetaRef as `op` right now in order to minimize changes.
This brings up a bigger long term question of how much we should rely on the meta table outside of things such as the constants or other "unprintable" values. I realize with my tokenization approach we could eagerly expand the meta references but this requires that we have the full program on hand in order to successfully parse any program fragment.
Would love to get some more thoughts and feedback cc @MarisaKirisame @joshpoll @tqchen @zhiics @electriclilies @mbrookhart @junrushao1994.
FYI cc @tmoreau89
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463858441
##########
File path: src/parser/meta_ref.cc
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file src/parser/meta_ref.cc
+ * \brief An operator which allows forward referencing a yet-to-be parsed meta table reference.
+ */
+
+#include "./meta_ref.h"
+
+#include <topi/elemwise.h>
+#include <tvm/relay/expr_functor.h>
+#include <tvm/relay/op.h>
+#include <tvm/relay/op_attr_types.h>
+#include <tvm/relay/transform.h>
+
+namespace tvm {
+namespace parser {
+
+using tvm::relay::transform::CreateFunctionPass;
+using tvm::transform::PassContext;
+
+TVM_REGISTER_NODE_TYPE(MetaRefAttrs);
Review comment:
I think this is a taste thing, I would rather have more files that are clearly delinated then put a lot of random things in the same file. Even if they are related the sheer amount of code in many files often confuses new TVM contributors as you might be wanting to just show one algorithm in the file, but there are a lot of support data strcutures and functions inlined into the file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463855404
##########
File path: src/parser/parser.cc
##########
@@ -542,39 +496,60 @@ class Parser {
*/
template <typename T>
Array<T> ParseSequence(TokenType start, TokenType sep, TokenType stop, std::function<T()> parse,
- std::function<void()> before_stop = nullptr) {
+ std::function<bool()> before_stop = nullptr) {
+ DLOG(INFO) << "Parser::ParseSequence: start=" << start << "sep=" << sep << "stop=" << stop;
Review comment:
We could potentially use the diagnostics to log some information, but I think that is a bigger change that requires more design, my goal is to first convert all user visible errors. Happy to revisit this in a few PRs as the current changes are already roughly 4-6kloc of code and I still haven't even converted the pass manager or pass context.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tkonolige commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r461922365
##########
File path: src/parser/meta_ref.cc
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file src/parser/meta_ref.cc
+ * \brief An operator which allows forward referencing a yet-to-be parsed meta table reference.
+ */
+
+#include <tvm/relay/op.h>
+#include <tvm/relay/expr_functor.h>
+#include <tvm/relay/transform.h>
+#include <tvm/relay/op_attr_types.h>
+#include <topi/elemwise.h>
+
+#include "./meta_ref.h"
+
+namespace tvm {
+namespace parser {
+
+using tvm::transform::PassContext;
+using tvm::relay::transform::CreateFunctionPass;
+
+TVM_REGISTER_NODE_TYPE(MetaRefAttrs);
+
+bool MetaRefRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ LOG(FATAL) << "need to expand before type checking";
+ return true;
+}
+
+RELAY_REGISTER_OP("parser.MetaRef")
+ .describe(R"code(A reference into the meta table.)code" TVM_ADD_FILELINE)
+ .set_attrs_type<MetaRefAttrs>()
+ .set_num_inputs(0)
+ .set_support_level(10)
+ .add_type_rel("MetaRef", MetaRefRel)
+ .set_attr<TOpIsStateful>("TOpIsStateful", false)
+ .set_attr<TNonComputational>("TNonComputational", true);
+
+Expr MetaRef(std::string type_key, uint64_t node_index) {
+ static const Op& op = Op::Get("parser.MetaRef");
+ auto attrs = make_object<MetaRefAttrs>();
+ attrs->node_type_key = tvm::String(type_key);
+ attrs->node_index = node_index;
+ return Call(op, {}, Attrs(attrs), {});
+}
+
+
+// class MetaRefAttrExpander : AttrFunctor<ObjectRef(const ObjectRef& n)> {
Review comment:
Commented on purpose?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r462106658
##########
File path: src/parser/meta_ref.h
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file meta_ref.h
+ * \brief A reference into the metadata section of the Relay text format.
+ */
+
+#ifndef TVM_PARSER_META_REF_H_
+#define TVM_PARSER_META_REF_H_
+
+
+#include <tvm/ir/attrs.h>
+#include <tvm/relay/expr.h>
+
+#include <string>
+
+namespace tvm {
+namespace parser {
+
+using namespace relay;
+
+using MetaTable = Map<String, Array<ObjectRef>>;
+
+/*!
+ * \brief Options for allocating storage.
+ */
+struct MetaRefAttrs : public tvm::AttrsNode<MetaRefAttrs> {
+ tvm::String node_type_key;
+ uint64_t node_index;
+
+ TVM_DECLARE_ATTRS(MetaRefAttrs, "relay.attrs.MetaRefAttrs") {
+ TVM_ATTR_FIELD(node_type_key)
+ .describe("The type_key representing the type of the node referenced.");
+ TVM_ATTR_FIELD(node_index).describe("The index into the type specific node array.");
+ }
+};
+
+/*! \brief A reference to a "meta-expression".
+ *
+ * In the text format we allow referencing metadata which
+ * uses a compact serialization that proceeds the main
+ * program body.
+ *
+ * We can reference this table using an expression of
+ * the form `meta[Type][index]`.
+ *
+ * We must later resolve these references to actual in-memory
+ * AST nodes but this requires first parsing the full program
+ * then expanding these temporary AST nodes into their corresponding
+ * nodes.
+ *
+ * For example the nth large constant will be pretty-printed as meta[relay.Constant][n]
+ * with its compact binary serialization residing in the metadata section at the end
+ * of the program.
+ *
+ * \param type_key The type key of the object in the meta section.
+ * \param kind The index into that subfield.
Review comment:
kind -> node_index
##########
File path: src/parser/diagnostic.h
##########
@@ -138,8 +62,73 @@ struct Diagnostic {
/*! \brief The diagnostic message. */
std::string message;
+ /*! \brief A diagnostic for a single character token. */
Diagnostic(int line, int column, const std::string& message)
- : level(DiagnosticLevel::Error), span(SourceName(), line, column), message(message) {}
+ : level(DiagnosticLevel::Error), span(SourceName(), line, column, line, column + 1), message(message) {}
+
+ Diagnostic(DiagnosticLevel level, Span span, const std::string& message)
+ : level(level), span(span), message(message) {}
+};
+
+/*!
+ * \brief A wrapper around std::stringstream to build a diagnostic.
+ *
+ * \code
+ *
+ * void ReportError(const Error& err);
+ *
+ * void Test(int number) {
+ * // Use error reporter to construct an error.
+ * ReportError(ErrorBuilder() << "This is an error number=" << number);
+ * }
+ *
+ * \endcode
+ */
+struct DiagnosticBuilder {
+ public:
+ /*! \brief The level. */
+ DiagnosticLevel level;
+
+ /*! \brief The source name. */
+ SourceName source_name;
+
+ /*! \brief The span of the diagnostic. */
+ Span span;
+
+ /*! \brief The line number. */
+ int line;
Review comment:
I am sorry! I am little confused here, may be i missed something.
Can you please help me understand, why we need line, column, end_line & end_column again, which we already maintain inside the Span object ?
The usual design i think would be ObjectBuilder --> Object .
##########
File path: python/tvm/parser/__init__.py
##########
@@ -24,4 +24,5 @@ def parse_expr(source):
return _ffi_api.ParseExpr("string", source)
def fromtext(source, source_name="from_string"):
+ # TODO(@tqchen): currently we have to invoke `str` which dramatically reduces performance.
Review comment:
Would you please help me understand this comment. Why you need to call str() if it is already a string object in python.
And if it is TVM::Runtime::String object, then much overloading is already supported to do the conversion.
Are you facing any error here?
##########
File path: include/tvm/parser/source_map.h
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.
+ */
+
+#ifndef TVM_PARSER_SOURCE_MAP_H_
+#define TVM_PARSER_SOURCE_MAP_H_
+/*!
+ * \file source_map.h
+ * \brief A map from source names to source code.
+ */
+#include <tvm/runtime/packed_func.h>
+#include <tvm/runtime/registry.h>
+#include <tvm/ir/span.h>
+
+#include <fstream>
+#include <string>
+
+namespace tvm {
+namespace parser {
+
+
+/*! \brief A program source in any language.
+ *
+ * Could represent the source from an ML framework or the internal
+ * source of a TVM program.
+ */
+struct Source {
+ /*! \brief The raw source. */
+ std::string source;
+ /*! \brief A mapping of line breaks into the raw source. */
+ std::vector<std::pair<int, int>> line_map;
+
+ /*! \brief An empty source. */
+ Source() : source(), line_map() {}
+
+ /*! \brief Construct a source from a string. */
+ TVM_DLL explicit Source(const std::string& source);
+
+ TVM_DLL Source(const Source& source) : source(source.source), line_map(source.line_map) {}
+
+ /*! \brief Generate an error message at a specific line and column with the
+ * annotated message.
+ *
+ * The error is written directly to the `out` std::ostream.
+ *
+ * \param out The output ostream.
+ * \param line The line at which to report a diagnostic.
+ * \param line The column at which to report a diagnostic.
+ * \param msg The message to attach.
+ */
+ TVM_DLL void ReportAt(std::ostream& out, int line, int column, const std::string& msg) const;
Review comment:
I think it would be good to use a TVM defined object to redirect the logs/debugs messages, rather than using ostream directly.
It helps to enable user to have a choice at later point in time, whether someone wants redirect logs to default screen or some files, or add some additional filters on top of it. And more importantly it is easily up-gradable.
Please let me know in case my point is not clear.
##########
File path: include/tvm/ir/span.h
##########
@@ -85,16 +85,26 @@ class SpanNode : public Object {
int line;
Review comment:
I believe what you are trying to achieve here is the range for line[begin, end], column[begin, end].
nit: May be we can rename the variables to reflect the same.
##########
File path: tests/python/relay/test_ir_parser.py
##########
@@ -868,48 +883,60 @@ def test_extern_adt_defn():
extern_def = relay.TypeData(extern_var, [typ_var], [])
mod[extern_var] = extern_def
- assert_parses_as(
+ assert_parse_module_as(
"""
extern type T[A]
""",
mod
)
+
+@pytest.mark.skip("not yet tested on parser 2.0")
def test_import_grad():
mod = tvm.IRModule()
mod.import_from_std("gradient.rly")
+# hiearchy id, i.e parse nn.conv2d
+# do with multiple levels
+#
+# call attributes not correctly parsing
+# convert error from attribute construction to real error message
+# lexing issue with projection of graph variables
+
+# def test_hierarchical_identifiers():
+# assert False
+
+def test_resnet():
+ mod, params = relay.testing.resnet.get_workload()
+ text = str(mod.astext())
+ parsed_mod = parse_module(text)
+ tvm.ir.assert_structural_equal(mod, parsed_mod)
+
+def inline_params(mod, params):
+ main_fn = mod["main"]
+ str_to_var = {}
+ for param in main_fn.params:
+ str_to_var[param.name_hint] = param
+
+ bind_map = {}
+ for param in params:
+ bind_map[str_to_var[param]] = relay.const(params[param])
+
+ body = relay.bind(main_fn.body, bind_map)
+ main_fn = relay.Function(relay.analysis.free_vars(body), body)
+ mod["main_fn"] = main_fn
+ return mod
+
+def test_resnet_inlined_params():
+ mod, params = relay.testing.resnet.get_workload()
+ print("here")
+ mod = inline_params(mod, params)
+ print("here")
+ text = str(mod.astext())
+ print("here")
+ parsed_mod = parse_module(text)
+ print("here")
+ tvm.ir.assert_structural_equal(mod, parsed_mod)
+ print("here")
+
if __name__ == "__main__":
- test_graph()
Review comment:
All test cases are removed ? If it is just a temp code, then this comment can act as reminder before merging :)
##########
File path: include/tvm/parser/source_map.h
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.
+ */
+
+#ifndef TVM_PARSER_SOURCE_MAP_H_
+#define TVM_PARSER_SOURCE_MAP_H_
+/*!
+ * \file source_map.h
+ * \brief A map from source names to source code.
+ */
+#include <tvm/runtime/packed_func.h>
+#include <tvm/runtime/registry.h>
+#include <tvm/ir/span.h>
+
+#include <fstream>
+#include <string>
+
+namespace tvm {
+namespace parser {
+
+
+/*! \brief A program source in any language.
+ *
+ * Could represent the source from an ML framework or the internal
+ * source of a TVM program.
+ */
+struct Source {
+ /*! \brief The raw source. */
+ std::string source;
+ /*! \brief A mapping of line breaks into the raw source. */
+ std::vector<std::pair<int, int>> line_map;
+
+ /*! \brief An empty source. */
+ Source() : source(), line_map() {}
+
+ /*! \brief Construct a source from a string. */
+ TVM_DLL explicit Source(const std::string& source);
+
+ TVM_DLL Source(const Source& source) : source(source.source), line_map(source.line_map) {}
+
+ /*! \brief Generate an error message at a specific line and column with the
+ * annotated message.
+ *
+ * The error is written directly to the `out` std::ostream.
+ *
+ * \param out The output ostream.
+ * \param line The line at which to report a diagnostic.
+ * \param line The column at which to report a diagnostic.
+ * \param msg The message to attach.
+ */
+ TVM_DLL void ReportAt(std::ostream& out, int line, int column, const std::string& msg) const;
+};
+
+/*!
+ * \brief A mapping from a unique source name to source fragment.
+ */
+class SourceMap;
Review comment:
nit: This class i think is more suitable if placed in span.h.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#issuecomment-671439329
@jroesch please followup to address the CI error
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463206316
##########
File path: src/parser/parser.cc
##########
@@ -542,39 +496,60 @@ class Parser {
*/
template <typename T>
Array<T> ParseSequence(TokenType start, TokenType sep, TokenType stop, std::function<T()> parse,
- std::function<void()> before_stop = nullptr) {
+ std::function<bool()> before_stop = nullptr) {
+ DLOG(INFO) << "Parser::ParseSequence: start=" << start << "sep=" << sep << "stop=" << stop;
Review comment:
Why not use DLOG? if anything we should be increasing logging in TVM not removing it. There has been more talk of doing increased structured logging after I finish doing error reporting but there is no RFC at the current time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] MarisaKirisame commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
MarisaKirisame commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463954217
##########
File path: include/tvm/ir/span.h
##########
@@ -85,16 +85,26 @@ class SpanNode : public Object {
int line;
Review comment:
I think it should be begin_line, end_line, begin_column, end_column, as both values are related. Maybe even make a small range struct.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbrookhart commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r465838414
##########
File path: python/tvm/error.py
##########
@@ -121,3 +121,7 @@ class OpAttributeUnImplemented(OpError, NotImplementedError):
"Attribute {} is not supported in operator {}".format(
attr_name, op_name))
"""
+
+@register_error
+class DiagnosticError(TVMError):
Review comment:
Docstring instead of pass?
##########
File path: src/parser/parser.cc
##########
@@ -1144,62 +1215,95 @@ class Parser {
}
// We need a zero-arity case for constructors.
- if (expr.as<ConstructorNode>()) {
- return Expr(Call(expr, {}));
- } else {
- return expr;
+ if (auto ctor_node = expr.as<ConstructorNode>()) {
+ if (ctor_node->inputs.size() == 0) {
+ return Expr(Call(expr, {}));
+ }
}
+
+ return expr;
});
}
+ Expr GetOp(const std::string& op_name, const Token& tok) {
+ try {
+ return Op::Get(op_name);
+ } catch (dmlc::Error e) {
+ this->diag_ctx->Emit(DiagnosticBuilder(DiagnosticLevel::Error, tok->span)
+ << "operator `" << op_name
+ << "` not found, perhaps you forgot to register it?");
+ return Expr();
+ }
+ }
+
Expr ParseAtomicExpr() {
- return ConsumeWhitespace<Expr>([this] {
+ DLOG(INFO) << "Parser::ParseAtomicExpr";
+ auto expr = ConsumeWhitespace<Expr>([this] {
auto next = Peek();
switch (next->token_type) {
case TokenType::Integer:
case TokenType::Float: {
Consume(next->token_type);
auto number = NumberToNDArray(next);
- Expr e = Constant(number);
+ Expr e = Constant(number, next->span);
return e;
}
case TokenType::Boolean: {
Consume(TokenType::Boolean);
int value = Downcast<tvm::Integer>(next->data);
auto boolean = BooleanToNDarray(value);
- Expr e = Constant(boolean);
+ Expr e = Constant(boolean, next->span);
return e;
}
+ // Parse a local of the form `%x`.
case TokenType::Local: {
Consume(TokenType::Local);
return Expr(LookupLocal(next));
}
+ // Parse a local of the form `@x`.
case TokenType::Global: {
auto string = next.ToString();
Consume(TokenType::Global);
auto global = global_names.Get(string);
if (!global) {
+ // TODO(@jroesch): fix global's needing span information
auto global_var = GlobalVar(string);
global_names.Add(string, global_var);
return Expr(global_var);
} else {
return Expr(global.value());
}
}
+ // Parse a local of the form `x`.
+ // Right now we fail to parse `x.y`.
case TokenType::Identifier: {
- auto string = next.ToString();
- Consume(TokenType::Identifier);
- auto ctor = ctors.Get(string);
+ auto ctor = ctors.Get(next.ToString());
if (ctor) {
+ Consume(TokenType::Identifier);
return Expr(ctor.value());
} else {
- return Expr(Op::Get(string));
+ auto idents = ParseHierName();
+ std::stringstream op_name;
+ int i = 0;
+ int periods = idents.size() - 1;
+ for (auto ident : idents) {
+ op_name << ident;
+ if (i < periods) {
+ op_name << ".";
+ i++;
+ }
+ }
Review comment:
Comment on this loop? Maybe make it a utility? It's a bit tricky to parse what this does from the surrounding code.
##########
File path: src/parser/parser.cc
##########
@@ -1231,14 +1335,38 @@ class Parser {
}
}
default: {
- std::stringstream msg;
- msg << "expected an expression found " << Pretty(next->token_type);
- diag_ctx.Emit({next->line, next->column, msg.str()});
- diag_ctx.Render(std::cout);
+ this->diag_ctx->EmitFatal(DiagnosticBuilder(DiagnosticLevel::Error, next->span)
+ << "expected an expression found "
+ << Pretty(next->token_type));
return Expr();
}
}
});
+
+ if (WhenMatch(TokenType::Period)) {
+ auto index = Match(TokenType::Integer).ToNumber();
+ expr = relay::TupleGetItem(expr, index);
+ }
+
+ return expr;
+ }
+
+ /*! \brief Parse a hierarchical name. */
+ Array<String> ParseHierName() {
Review comment:
Maybe expand this to ParseHierarchicalName? It's not a common shortening, took a while to figure out what the function name meant when I saw it in code.
##########
File path: src/parser/token.h
##########
@@ -85,6 +86,9 @@ enum TokenType {
Extern,
Match,
PartialMatch,
+ Metadata,
Review comment:
+1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463101025
##########
File path: src/parser/meta_ref.cc
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file src/parser/meta_ref.cc
+ * \brief An operator which allows forward referencing a yet-to-be parsed meta table reference.
+ */
+
+#include "./meta_ref.h"
+
+#include <topi/elemwise.h>
+#include <tvm/relay/expr_functor.h>
+#include <tvm/relay/op.h>
+#include <tvm/relay/op_attr_types.h>
+#include <tvm/relay/transform.h>
+
+namespace tvm {
+namespace parser {
+
+using tvm::relay::transform::CreateFunctionPass;
+using tvm::transform::PassContext;
+
+TVM_REGISTER_NODE_TYPE(MetaRefAttrs);
+
+bool MetaRefRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ LOG(FATAL) << "need to expand before type checking";
+ return true;
+}
+
+RELAY_REGISTER_OP("parser.MetaRef")
Review comment:
nit: As "parser.MetaRef" carry special meaning in the context, i think we can define this key at one place, and reuse it in all places. This helps if we want to change it at later point in time.
##########
File path: src/ir/span.cc
##########
@@ -61,18 +61,29 @@ TVM_REGISTER_NODE_TYPE(SourceNameNode)
return static_cast<const SourceNameNode*>(n)->name;
});
-Span::Span(SourceName source, int lineno, int col_offset) {
+Span::Span(SourceName source, int line, int column, int end_line, int end_column) {
auto n = make_object<SpanNode>();
n->source = std::move(source);
- n->line = lineno;
- n->column = col_offset;
+ n->line = line;
+ n->column = column;
+ n->end_line = end_line;
+ n->end_column = end_column;
data_ = std::move(n);
}
+Span Span::Merge(const Span& other) {
Review comment:
I understand the logic here clearly. But i failed to deduce the purpose or usability of this utility.
As per i understand, using this utility we want to have a common region or a union of regions.
But the logic here may work, if we work in a consecutive environment, which will not be the case i believe.
Would you please help me understand it better. TIA!
If we really need a tracing mechanism which would tell us a Span [Start Node, End Node]. Then i believe this utility should not be part of Span, it should be an independent one.
##########
File path: src/parser/meta_ref.cc
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file src/parser/meta_ref.cc
+ * \brief An operator which allows forward referencing a yet-to-be parsed meta table reference.
+ */
+
+#include "./meta_ref.h"
+
+#include <topi/elemwise.h>
+#include <tvm/relay/expr_functor.h>
+#include <tvm/relay/op.h>
+#include <tvm/relay/op_attr_types.h>
+#include <tvm/relay/transform.h>
+
+namespace tvm {
+namespace parser {
+
+using tvm::relay::transform::CreateFunctionPass;
+using tvm::transform::PassContext;
+
+TVM_REGISTER_NODE_TYPE(MetaRefAttrs);
+
+bool MetaRefRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ LOG(FATAL) << "need to expand before type checking";
+ return true;
+}
+
+RELAY_REGISTER_OP("parser.MetaRef")
+ .describe(R"code(A reference into the meta table.)code" TVM_ADD_FILELINE)
+ .set_attrs_type<MetaRefAttrs>()
+ .set_num_inputs(0)
+ .set_support_level(10)
+ .add_type_rel("MetaRef", MetaRefRel)
+ .set_attr<TOpIsStateful>("TOpIsStateful", false)
+ .set_attr<TNonComputational>("TNonComputational", true);
+
+Expr MetaRef(std::string type_key, uint64_t node_index) {
+ static const Op& op = Op::Get("parser.MetaRef");
+ auto attrs = make_object<MetaRefAttrs>();
+ attrs->node_type_key = tvm::String(type_key);
+ attrs->node_index = node_index;
+ return Call(op, {}, Attrs(attrs), {});
+}
+
+// class MetaRefAttrExpander : AttrFunctor<ObjectRef(const ObjectRef& n)> {
+// ObjectRef VisitAttrDefault_(const Object* node) final {
+
+// }
+// }
+
+struct MetaRefExpander : public ExprMutator {
+ MetaTable table;
+
+ explicit MetaRefExpander(const MetaTable& table) : table(table) {}
+
+ Expr VisitExpr_(const CallNode* call) final {
+ if (auto op_node = call->op.as<OpNode>()) {
Review comment:
Both if can be clubbed, unless future plan to expand the if else branch
##########
File path: src/parser/meta_ref.h
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file meta_ref.h
+ * \brief A reference into the metadata section of the Relay text format.
+ */
+
+#ifndef TVM_PARSER_META_REF_H_
+#define TVM_PARSER_META_REF_H_
+
+#include <tvm/ir/attrs.h>
+#include <tvm/relay/expr.h>
+#include <tvm/relay/function.h>
+
+#include <string>
+
+namespace tvm {
+namespace parser {
+
+using namespace relay;
+
+using MetaTable = Map<String, Array<ObjectRef>>;
+
+/*!
+ * \brief Options for allocating storage.
+ */
+struct MetaRefAttrs : public tvm::AttrsNode<MetaRefAttrs> {
+ tvm::String node_type_key;
+ uint64_t node_index;
+
+ TVM_DECLARE_ATTRS(MetaRefAttrs, "relay.attrs.MetaRefAttrs") {
+ TVM_ATTR_FIELD(node_type_key)
+ .describe("The type_key representing the type of the node referenced.");
+ TVM_ATTR_FIELD(node_index).describe("The index into the type specific node array.");
+ }
+};
+
+/*! \brief A reference to a "meta-expression".
+ *
+ * In the text format we allow referencing metadata which
+ * uses a compact serialization that proceeds the main
+ * program body.
+ *
+ * We can reference this table using an expression of
+ * the form `meta[Type][index]`.
+ *
+ * We must later resolve these references to actual in-memory
+ * AST nodes but this requires first parsing the full program
+ * then expanding these temporary AST nodes into their corresponding
+ * nodes.
+ *
+ * For example the nth large constant will be pretty-printed as meta[relay.Constant][n]
+ * with its compact binary serialization residing in the metadata section at the end
+ * of the program.
+ *
+ * \param type_key The type key of the object in the meta section.
+ * \param kind The index into that subfield.
+ * \returns The meta table reference.
+ */
+Expr MetaRef(std::string type_key, uint64_t node_index);
+
+relay::Function ExpandMetaRefs(const MetaTable& meta_table, const relay::Function& mod);
Review comment:
mod -> func ?
##########
File path: src/parser/diagnostic.h
##########
@@ -138,8 +61,77 @@ struct Diagnostic {
/*! \brief The diagnostic message. */
std::string message;
+ /*! \brief A diagnostic for a single character token. */
Diagnostic(int line, int column, const std::string& message)
- : level(DiagnosticLevel::Error), span(SourceName(), line, column), message(message) {}
+ : level(DiagnosticLevel::Error),
+ span(SourceName(), line, column, line, column + 1),
+ message(message) {}
+
+ Diagnostic(DiagnosticLevel level, Span span, const std::string& message)
+ : level(level), span(span), message(message) {}
+};
+
+/*!
+ * \brief A wrapper around std::stringstream to build a diagnostic.
+ *
+ * \code
+ *
+ * void ReportError(const Error& err);
+ *
+ * void Test(int number) {
+ * // Use error reporter to construct an error.
+ * ReportError(ErrorBuilder() << "This is an error number=" << number);
+ * }
+ *
+ * \endcode
+ */
+struct DiagnosticBuilder {
Review comment:
maybe class ?
##########
File path: include/tvm/relay/adt.h
##########
@@ -310,7 +310,7 @@ class Match : public Expr {
* \param clauses The clauses for matching.
* \param complete Indicate if this match is complete.
*/
- TVM_DLL Match(Expr data, tvm::Array<Clause> clauses, bool complete = true);
+ TVM_DLL Match(Expr data, tvm::Array<Clause> clauses, bool complete = true, Span span = Span());
Review comment:
Please add doc for span param, same is applicable to all constructors added.
##########
File path: src/parser/parser.cc
##########
@@ -427,8 +381,8 @@ class Parser {
Var LookupLocal(const Token& local) {
auto var = this->expr_scopes.Lookup(local.ToString());
if (!var.defined()) {
- diag_ctx.Emit(
- {local->line, local->column, "this local variable has not been previously declared"});
+ diag_ctx->Emit({DiagnosticLevel::Error, local->span,
Review comment:
May be we can standardize usage of DiagnosticBuilder instead of Diagnostic struct directly.
It will create unnecessary confusion in future and overhead of maintenance, in case of any design change.
##########
File path: src/parser/meta_ref.cc
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file src/parser/meta_ref.cc
+ * \brief An operator which allows forward referencing a yet-to-be parsed meta table reference.
+ */
+
+#include "./meta_ref.h"
+
+#include <topi/elemwise.h>
+#include <tvm/relay/expr_functor.h>
+#include <tvm/relay/op.h>
+#include <tvm/relay/op_attr_types.h>
+#include <tvm/relay/transform.h>
+
+namespace tvm {
+namespace parser {
+
+using tvm::relay::transform::CreateFunctionPass;
+using tvm::transform::PassContext;
+
+TVM_REGISTER_NODE_TYPE(MetaRefAttrs);
Review comment:
Should we club all these pieces in one file like Op reg and FunctionPass ?
I think if we keep in respective folder it would be good. I am not too sure, we can get other's opinion too.
##########
File path: src/parser/parser.cc
##########
@@ -542,39 +496,60 @@ class Parser {
*/
template <typename T>
Array<T> ParseSequence(TokenType start, TokenType sep, TokenType stop, std::function<T()> parse,
- std::function<void()> before_stop = nullptr) {
+ std::function<bool()> before_stop = nullptr) {
+ DLOG(INFO) << "Parser::ParseSequence: start=" << start << "sep=" << sep << "stop=" << stop;
Review comment:
Should we still use DLOG ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463857977
##########
File path: src/parser/meta_ref.cc
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file src/parser/meta_ref.cc
+ * \brief An operator which allows forward referencing a yet-to-be parsed meta table reference.
+ */
+
+#include "./meta_ref.h"
+
+#include <topi/elemwise.h>
+#include <tvm/relay/expr_functor.h>
+#include <tvm/relay/op.h>
+#include <tvm/relay/op_attr_types.h>
+#include <tvm/relay/transform.h>
+
+namespace tvm {
+namespace parser {
+
+using tvm::relay::transform::CreateFunctionPass;
+using tvm::transform::PassContext;
+
+TVM_REGISTER_NODE_TYPE(MetaRefAttrs);
+
+bool MetaRefRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ LOG(FATAL) << "need to expand before type checking";
+ return true;
+}
+
+RELAY_REGISTER_OP("parser.MetaRef")
+ .describe(R"code(A reference into the meta table.)code" TVM_ADD_FILELINE)
+ .set_attrs_type<MetaRefAttrs>()
+ .set_num_inputs(0)
+ .set_support_level(10)
+ .add_type_rel("MetaRef", MetaRefRel)
+ .set_attr<TOpIsStateful>("TOpIsStateful", false)
+ .set_attr<TNonComputational>("TNonComputational", true);
+
+Expr MetaRef(std::string type_key, uint64_t node_index) {
+ static const Op& op = Op::Get("parser.MetaRef");
+ auto attrs = make_object<MetaRefAttrs>();
+ attrs->node_type_key = tvm::String(type_key);
+ attrs->node_index = node_index;
+ return Call(op, {}, Attrs(attrs), {});
+}
+
+// class MetaRefAttrExpander : AttrFunctor<ObjectRef(const ObjectRef& n)> {
+// ObjectRef VisitAttrDefault_(const Object* node) final {
+
+// }
+// }
+
+struct MetaRefExpander : public ExprMutator {
+ MetaTable table;
+
+ explicit MetaRefExpander(const MetaTable& table) : table(table) {}
+
+ Expr VisitExpr_(const CallNode* call) final {
+ if (auto op_node = call->op.as<OpNode>()) {
Review comment:
As per my discussion above I"m not sure this metaref is the best way to do it, would like feedback on overall approach.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r467340470
##########
File path: src/parser/parser.cc
##########
@@ -1231,14 +1335,38 @@ class Parser {
}
}
default: {
- std::stringstream msg;
- msg << "expected an expression found " << Pretty(next->token_type);
- diag_ctx.Emit({next->line, next->column, msg.str()});
- diag_ctx.Render(std::cout);
+ this->diag_ctx->EmitFatal(DiagnosticBuilder(DiagnosticLevel::Error, next->span)
+ << "expected an expression found "
+ << Pretty(next->token_type));
return Expr();
}
}
});
+
+ if (WhenMatch(TokenType::Period)) {
+ auto index = Match(TokenType::Integer).ToNumber();
+ expr = relay::TupleGetItem(expr, index);
+ }
+
+ return expr;
+ }
+
+ /*! \brief Parse a hierarchical name. */
+ Array<String> ParseHierName() {
Review comment:
Yeah I will document this, was quickly hacking up with Jason on the stream in order to unblock, worth coming back to it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] MarisaKirisame commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
MarisaKirisame commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r462187462
##########
File path: tests/python/relay/test_ir_parser2.py
##########
@@ -1,891 +0,0 @@
-# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
Why are you removing test? If it doesnt work yet we should comment them and fix it in next pr.
##########
File path: src/ir/span.cc
##########
@@ -61,18 +61,30 @@ TVM_REGISTER_NODE_TYPE(SourceNameNode)
return static_cast<const SourceNameNode*>(n)->name;
});
-Span::Span(SourceName source, int lineno, int col_offset) {
+Span::Span(SourceName source, int line, int column, int end_line, int end_column) {
Review comment:
```suggestion
Span::Span(SourceName source, int line, int end_line, int column, int end_column) {
```
##########
File path: src/parser/meta_ref.cc
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file src/parser/meta_ref.cc
+ * \brief An operator which allows forward referencing a yet-to-be parsed meta table reference.
+ */
+
+#include <tvm/relay/op.h>
+#include <tvm/relay/expr_functor.h>
+#include <tvm/relay/transform.h>
+#include <tvm/relay/op_attr_types.h>
+#include <topi/elemwise.h>
+
+#include "./meta_ref.h"
+
+namespace tvm {
+namespace parser {
+
+using tvm::transform::PassContext;
+using tvm::relay::transform::CreateFunctionPass;
+
+TVM_REGISTER_NODE_TYPE(MetaRefAttrs);
+
+bool MetaRefRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ LOG(FATAL) << "need to expand before type checking";
+ return true;
+}
+
+RELAY_REGISTER_OP("parser.MetaRef")
+ .describe(R"code(A reference into the meta table.)code" TVM_ADD_FILELINE)
+ .set_attrs_type<MetaRefAttrs>()
+ .set_num_inputs(0)
+ .set_support_level(10)
+ .add_type_rel("MetaRef", MetaRefRel)
+ .set_attr<TOpIsStateful>("TOpIsStateful", false)
+ .set_attr<TNonComputational>("TNonComputational", true);
+
+Expr MetaRef(std::string type_key, uint64_t node_index) {
+ static const Op& op = Op::Get("parser.MetaRef");
+ auto attrs = make_object<MetaRefAttrs>();
+ attrs->node_type_key = tvm::String(type_key);
+ attrs->node_index = node_index;
+ return Call(op, {}, Attrs(attrs), {});
+}
+
+
+// class MetaRefAttrExpander : AttrFunctor<ObjectRef(const ObjectRef& n)> {
+// ObjectRef VisitAttrDefault_(const Object* node) final {
+
+// }
+// }
+
+struct MetaRefExpander : public ExprMutator {
+ MetaTable table;
+
+ MetaRefExpander(const MetaTable& table) : table(table) {}
+
+ Expr VisitExpr_(const CallNode* call) final {
+ if (auto op_node = call->op.as<OpNode>()) {
+ if (op_node->name == "parser.MetaRef") {
+ auto meta_attrs = call->attrs.as<MetaRefAttrs>();
+ CHECK(meta_attrs) << "an internal error has occurred";
+ auto nodes = table.at(meta_attrs->node_type_key);
+ CHECK_LT(meta_attrs->node_index, nodes.size());
+ return Downcast<Expr>(nodes[meta_attrs->node_index]);
+ }
+ }
+
+ return ExprMutator::VisitExpr_(call);
+ }
+};
+
+Function ExpandMetaRefs(const MetaTable& meta_table, const relay::Function& func) {
+ MetaRefExpander expander(meta_table);
+ return Downcast<Function>(expander.VisitExpr(func));
+}
+
+IRModule ExpandMetaRefs(const MetaTable& meta_table, const IRModule& mod) {
+ auto pass = CreateFunctionPass([&](Function func, IRModule module, PassContext ctx) {
+ return ExpandMetaRefs(meta_table, func);
+ }, 1337, "ExpandMetaRefs", {});
Review comment:
explain the magic number
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463857712
##########
File path: src/parser/diagnostic.h
##########
@@ -138,8 +61,77 @@ struct Diagnostic {
/*! \brief The diagnostic message. */
std::string message;
+ /*! \brief A diagnostic for a single character token. */
Diagnostic(int line, int column, const std::string& message)
- : level(DiagnosticLevel::Error), span(SourceName(), line, column), message(message) {}
+ : level(DiagnosticLevel::Error),
+ span(SourceName(), line, column, line, column + 1),
+ message(message) {}
+
+ Diagnostic(DiagnosticLevel level, Span span, const std::string& message)
+ : level(level), span(span), message(message) {}
+};
+
+/*!
+ * \brief A wrapper around std::stringstream to build a diagnostic.
+ *
+ * \code
+ *
+ * void ReportError(const Error& err);
+ *
+ * void Test(int number) {
+ * // Use error reporter to construct an error.
+ * ReportError(ErrorBuilder() << "This is an error number=" << number);
+ * }
+ *
+ * \endcode
+ */
+struct DiagnosticBuilder {
Review comment:
yeah TQ and others's like class, I thing C++ privacy rules aren't very good and mostly an annoyance so I tend to use structs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r465335674
##########
File path: src/parser/meta_ref.cc
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file src/parser/meta_ref.cc
+ * \brief An operator which allows forward referencing a yet-to-be parsed meta table reference.
+ */
+
+#include <tvm/relay/op.h>
+#include <tvm/relay/expr_functor.h>
+#include <tvm/relay/transform.h>
+#include <tvm/relay/op_attr_types.h>
+#include <topi/elemwise.h>
+
+#include "./meta_ref.h"
+
+namespace tvm {
+namespace parser {
+
+using tvm::transform::PassContext;
+using tvm::relay::transform::CreateFunctionPass;
+
+TVM_REGISTER_NODE_TYPE(MetaRefAttrs);
+
+bool MetaRefRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ LOG(FATAL) << "need to expand before type checking";
+ return true;
+}
+
+RELAY_REGISTER_OP("parser.MetaRef")
+ .describe(R"code(A reference into the meta table.)code" TVM_ADD_FILELINE)
+ .set_attrs_type<MetaRefAttrs>()
+ .set_num_inputs(0)
+ .set_support_level(10)
+ .add_type_rel("MetaRef", MetaRefRel)
+ .set_attr<TOpIsStateful>("TOpIsStateful", false)
+ .set_attr<TNonComputational>("TNonComputational", true);
+
+Expr MetaRef(std::string type_key, uint64_t node_index) {
+ static const Op& op = Op::Get("parser.MetaRef");
+ auto attrs = make_object<MetaRefAttrs>();
+ attrs->node_type_key = tvm::String(type_key);
+ attrs->node_index = node_index;
+ return Call(op, {}, Attrs(attrs), {});
+}
+
+
+// class MetaRefAttrExpander : AttrFunctor<ObjectRef(const ObjectRef& n)> {
+// ObjectRef VisitAttrDefault_(const Object* node) final {
+
+// }
+// }
+
+struct MetaRefExpander : public ExprMutator {
+ MetaTable table;
+
+ MetaRefExpander(const MetaTable& table) : table(table) {}
+
+ Expr VisitExpr_(const CallNode* call) final {
+ if (auto op_node = call->op.as<OpNode>()) {
+ if (op_node->name == "parser.MetaRef") {
+ auto meta_attrs = call->attrs.as<MetaRefAttrs>();
+ CHECK(meta_attrs) << "an internal error has occurred";
+ auto nodes = table.at(meta_attrs->node_type_key);
+ CHECK_LT(meta_attrs->node_index, nodes.size());
+ return Downcast<Expr>(nodes[meta_attrs->node_index]);
+ }
+ }
+
+ return ExprMutator::VisitExpr_(call);
+ }
+};
+
+Function ExpandMetaRefs(const MetaTable& meta_table, const relay::Function& func) {
+ MetaRefExpander expander(meta_table);
+ return Downcast<Function>(expander.VisitExpr(func));
+}
+
+IRModule ExpandMetaRefs(const MetaTable& meta_table, const IRModule& mod) {
+ auto pass = CreateFunctionPass([&](Function func, IRModule module, PassContext ctx) {
+ return ExpandMetaRefs(meta_table, func);
+ }, 1337, "ExpandMetaRefs", {});
Review comment:
This pass will never be scheduled by pass manager so I gave it a nonsense optimization level. I still don't really believe in optimization levels.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463856280
##########
File path: include/tvm/relay/adt.h
##########
@@ -310,7 +310,7 @@ class Match : public Expr {
* \param clauses The clauses for matching.
* \param complete Indicate if this match is complete.
*/
- TVM_DLL Match(Expr data, tvm::Array<Clause> clauses, bool complete = true);
+ TVM_DLL Match(Expr data, tvm::Array<Clause> clauses, bool complete = true, Span span = Span());
Review comment:
I think this is fixed everywhere.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463854884
##########
File path: src/parser/parser.cc
##########
@@ -427,8 +381,8 @@ class Parser {
Var LookupLocal(const Token& local) {
auto var = this->expr_scopes.Lookup(local.ToString());
if (!var.defined()) {
- diag_ctx.Emit(
- {local->line, local->column, "this local variable has not been previously declared"});
+ diag_ctx->Emit({DiagnosticLevel::Error, local->span,
Review comment:
I want the interface to be defined over diagnostics, the builder is just sugar for making it cleaner to report errors. I don't really want to freeze anything yet given we haven't converted passes to using this yet and have not seen all use cases. I changed the builder to be hidden you can now use `Diagnostic::Error(span) << msg` in order to build diagnostics.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] junrushao1994 commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463897900
##########
File path: src/parser/tokenizer.h
##########
@@ -32,13 +33,25 @@
#include <unordered_map>
#include <vector>
+#include "./meta_ref.h"
#include "./token.h"
namespace tvm {
namespace parser {
using namespace runtime;
+// trim from start (in place)
+static inline void ltrim(std::string& s) { // NOLINT(*)
+ s.erase(s.begin(), std::find_if(s.begin(), s.end(), [](int ch) { return !std::isspace(ch); }));
Review comment:
Do you prefer to use IsWhitespace we defined below?
##########
File path: include/tvm/ir/span.h
##########
@@ -85,16 +85,26 @@ class SpanNode : public Object {
int line;
Review comment:
Yeah I think I agree with @ANSHUMAN87. Maybe its better to use "begin_line", "begin_column", "end_line", "end_column"?
##########
File path: include/tvm/parser/source_map.h
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.
+ */
+
+#ifndef TVM_PARSER_SOURCE_MAP_H_
+#define TVM_PARSER_SOURCE_MAP_H_
+/*!
+ * \file source_map.h
+ * \brief A map from source names to source code.
+ */
Review comment:
We should move these 4 lines above the include guard
##########
File path: include/tvm/parser/source_map.h
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.
+ */
+
+#ifndef TVM_PARSER_SOURCE_MAP_H_
+#define TVM_PARSER_SOURCE_MAP_H_
+/*!
+ * \file source_map.h
+ * \brief A map from source names to source code.
+ */
+#include <tvm/ir/span.h>
+#include <tvm/runtime/packed_func.h>
+#include <tvm/runtime/registry.h>
+
+#include <fstream>
+#include <string>
+#include <utility>
+#include <vector>
+
+namespace tvm {
+namespace parser {
+
+/*! \brief A program source in any language.
+ *
+ * Could represent the source from an ML framework or the internal
+ * source of a TVM program.
+ */
+struct Source {
Review comment:
if it is not like POD aggregation, I personally prefer to use class instead
```suggestion
class Source {
public:
```
##########
File path: src/parser/diagnostic.h
##########
@@ -138,8 +61,77 @@ struct Diagnostic {
/*! \brief The diagnostic message. */
std::string message;
+ /*! \brief A diagnostic for a single character token. */
Diagnostic(int line, int column, const std::string& message)
- : level(DiagnosticLevel::Error), span(SourceName(), line, column), message(message) {}
+ : level(DiagnosticLevel::Error),
+ span(SourceName(), line, column, line, column + 1),
+ message(message) {}
+
+ Diagnostic(DiagnosticLevel level, Span span, const std::string& message)
+ : level(level), span(span), message(message) {}
+};
+
+/*!
+ * \brief A wrapper around std::stringstream to build a diagnostic.
+ *
+ * \code
+ *
+ * void ReportError(const Error& err);
+ *
+ * void Test(int number) {
+ * // Use error reporter to construct an error.
+ * ReportError(ErrorBuilder() << "This is an error number=" << number);
+ * }
+ *
+ * \endcode
+ */
+struct DiagnosticBuilder {
Review comment:
yeah i think there is no much difference between struct and class except for default access rules, but just a nitpick, people usually use struct for POD/aggregation like those without methods, and in other cases class is preferred.
##########
File path: include/tvm/relay/adt.h
##########
@@ -309,8 +309,9 @@ class Match : public Expr {
* \param data the input being deconstructed.
* \param clauses The clauses for matching.
* \param complete Indicate if this match is complete.
+ * \param span The span of the expression.
*/
- TVM_DLL Match(Expr data, tvm::Array<Clause> clauses, bool complete = true);
+ TVM_DLL Match(Expr data, tvm::Array<Clause> clauses, bool complete = true, Span span = Span());
Review comment:
Maybe we should think a bit about `Span(nullptr)` vs `Optional<Span>`?
##########
File path: include/tvm/parser/source_map.h
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.
+ */
+
+#ifndef TVM_PARSER_SOURCE_MAP_H_
+#define TVM_PARSER_SOURCE_MAP_H_
+/*!
+ * \file source_map.h
+ * \brief A map from source names to source code.
+ */
+#include <tvm/ir/span.h>
+#include <tvm/runtime/packed_func.h>
+#include <tvm/runtime/registry.h>
+
+#include <fstream>
+#include <string>
+#include <utility>
+#include <vector>
+
+namespace tvm {
+namespace parser {
+
+/*! \brief A program source in any language.
+ *
+ * Could represent the source from an ML framework or the internal
+ * source of a TVM program.
+ */
+struct Source {
+ /*! \brief The raw source. */
+ std::string source;
+ /*! \brief A mapping of line breaks into the raw source. */
+ std::vector<std::pair<int, int>> line_map;
+
+ /*! \brief An empty source. */
+ Source() : source(), line_map() {}
+
+ /*! \brief Construct a source from a string. */
+ TVM_DLL explicit Source(const std::string& source);
+
+ TVM_DLL Source(const Source& source) : source(source.source), line_map(source.line_map) {}
Review comment:
Shall we just use `= default`?
##########
File path: src/parser/diagnostic.h
##########
@@ -42,84 +43,6 @@
namespace tvm {
namespace parser {
-/*! \brief A program source in any language.
- *
- * Could represent the source from an ML framework or the internal
- * source of a TVM program.
- */
-struct Source {
- /*! \brief The raw source. */
- std::string source;
- /*! \brief A mapping of line breaks into the raw source. */
- std::vector<std::pair<int, int>> line_map;
-
- /*! \brief An empty source. */
- Source() : source(), line_map() {}
-
- /*! \brief Construct a source from a string. */
- explicit Source(const std::string& source) : source(source) {
- int index = 0;
- int length = 0;
- line_map.push_back({index, length});
- for (auto c : source) {
- if (c == '\n') {
- // Record the length of the line.
- line_map.back().second = length;
- // Bump past the newline.
- index += 1;
- // Record the start of the next line, and put placeholder for length.
- line_map.push_back({index, 0});
- // Reset length to zero.
- length = 0;
- } else {
- length += 1;
- index += 1;
- }
- }
- line_map.back().second = length;
- }
-
- Source(const Source& source) : source(source.source), line_map(source.line_map) {}
-
- /*! \brief Generate an error message at a specific line and column with the
- * annotated message.
- *
- * The error is written directly to the `out` std::ostream.
- *
- * \param out The output ostream.
- * \param line The line at which to report a diagnostic.
- * \param line The column at which to report a diagnostic.
- * \param msg The message to attach.
- */
- void ReportAt(std::ostream& out, int line, int column, const std::string& msg) const {
- CHECK(line - 1 <= static_cast<int64_t>(line_map.size()))
- << "requested line: " << (line - 1) << "line_map size: " << line_map.size()
- << "source: " << source;
-
- // Adjust for zero indexing, now have (line_start, line_length);
- auto range = line_map.at(line - 1);
- int line_start = range.first;
- int line_length = range.second;
- out << "file:" << line << ":" << column << ": parse error: " << msg << std::endl;
- out << " " << source.substr(line_start, line_length) << std::endl;
- out << " ";
- std::stringstream marker;
- for (int i = 1; i <= line_length; i++) {
- if (i == column) {
- marker << "^";
- } else if ((column - i) < 3) {
- marker << "~";
- } else if ((i - column) < 3) {
- marker << "~";
- } else {
- marker << " ";
- }
- }
- out << marker.str();
- out << std::endl;
- }
-};
-
/*! \brief The diagnostic level, controls the printing of the message. */
enum DiagnosticLevel {
Review comment:
Use enum class if possible
##########
File path: include/tvm/parser/source_map.h
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.
+ */
+
+#ifndef TVM_PARSER_SOURCE_MAP_H_
+#define TVM_PARSER_SOURCE_MAP_H_
+/*!
+ * \file source_map.h
+ * \brief A map from source names to source code.
+ */
+#include <tvm/ir/span.h>
+#include <tvm/runtime/packed_func.h>
+#include <tvm/runtime/registry.h>
+
+#include <fstream>
+#include <string>
+#include <utility>
+#include <vector>
+
+namespace tvm {
+namespace parser {
+
+/*! \brief A program source in any language.
+ *
+ * Could represent the source from an ML framework or the internal
+ * source of a TVM program.
+ */
+struct Source {
+ /*! \brief The raw source. */
+ std::string source;
+ /*! \brief A mapping of line breaks into the raw source. */
+ std::vector<std::pair<int, int>> line_map;
+
+ /*! \brief An empty source. */
+ Source() : source(), line_map() {}
+
+ /*! \brief Construct a source from a string. */
+ TVM_DLL explicit Source(const std::string& source);
+
+ TVM_DLL Source(const Source& source) : source(source.source), line_map(source.line_map) {}
+
+ /*! \brief Generate an error message at a specific line and column with the
+ * annotated message.
+ *
+ * The error is written directly to the `out` std::ostream.
+ *
+ * \param out The output ostream.
+ * \param span The span to report the error at.
+ * \param msg The message to attach.
+ *
+ */
+ // TODO(@jroesch): replace the ostream with an interface for rendering errors.
+ TVM_DLL void ReportAt(std::ostream& out, const Span& span, const std::string& msg) const;
+};
+
+/*!
+ * \brief A mapping from a unique source name to source fragment.
+ */
+class SourceMap;
+/*!
+ * \brief Stores locations in frontend source that generated a node.
+ */
+class SourceMapNode : public Object {
+ public:
+ /*! \brief The source mapping. */
+ Map<SourceName, tvm::String> source_map;
Review comment:
nit: not much difference tho
```suggestion
Map<SourceName, tvm::runtime::String> source_map;
```
##########
File path: src/parser/token.h
##########
@@ -85,6 +86,9 @@ enum TokenType {
Extern,
Match,
PartialMatch,
+ Metadata,
Review comment:
Use enum class if possible. Also, I think in Google style people prefix constants with a "k", e.g. kMetadata, kVersion
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r462751589
##########
File path: python/tvm/parser/__init__.py
##########
@@ -24,4 +24,5 @@ def parse_expr(source):
return _ffi_api.ParseExpr("string", source)
def fromtext(source, source_name="from_string"):
+ # TODO(@tqchen): currently we have to invoke `str` which dramatically reduces performance.
Review comment:
Got it! Thanks for clarification. I will also have a check on it.
##########
File path: include/tvm/parser/source_map.h
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.
+ */
+
+#ifndef TVM_PARSER_SOURCE_MAP_H_
+#define TVM_PARSER_SOURCE_MAP_H_
+/*!
+ * \file source_map.h
+ * \brief A map from source names to source code.
+ */
+#include <tvm/runtime/packed_func.h>
+#include <tvm/runtime/registry.h>
+#include <tvm/ir/span.h>
+
+#include <fstream>
+#include <string>
+
+namespace tvm {
+namespace parser {
+
+
+/*! \brief A program source in any language.
+ *
+ * Could represent the source from an ML framework or the internal
+ * source of a TVM program.
+ */
+struct Source {
+ /*! \brief The raw source. */
+ std::string source;
+ /*! \brief A mapping of line breaks into the raw source. */
+ std::vector<std::pair<int, int>> line_map;
+
+ /*! \brief An empty source. */
+ Source() : source(), line_map() {}
+
+ /*! \brief Construct a source from a string. */
+ TVM_DLL explicit Source(const std::string& source);
+
+ TVM_DLL Source(const Source& source) : source(source.source), line_map(source.line_map) {}
+
+ /*! \brief Generate an error message at a specific line and column with the
+ * annotated message.
+ *
+ * The error is written directly to the `out` std::ostream.
+ *
+ * \param out The output ostream.
+ * \param line The line at which to report a diagnostic.
+ * \param line The column at which to report a diagnostic.
+ * \param msg The message to attach.
+ */
+ TVM_DLL void ReportAt(std::ostream& out, int line, int column, const std::string& msg) const;
Review comment:
2.0 is indeed better. :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463857316
##########
File path: src/ir/span.cc
##########
@@ -61,18 +61,29 @@ TVM_REGISTER_NODE_TYPE(SourceNameNode)
return static_cast<const SourceNameNode*>(n)->name;
});
-Span::Span(SourceName source, int lineno, int col_offset) {
+Span::Span(SourceName source, int line, int column, int end_line, int end_column) {
auto n = make_object<SpanNode>();
n->source = std::move(source);
- n->line = lineno;
- n->column = col_offset;
+ n->line = line;
+ n->column = column;
+ n->end_line = end_line;
+ n->end_column = end_column;
data_ = std::move(n);
}
+Span Span::Merge(const Span& other) {
Review comment:
I converted the tokenizer to track spans as well and removed any use of raw column and/or row. Now the idea is my head is most tokens will start off as a single character or set of characters i.e (1, 1, 1, 2) or something like this. The tokens are consecutive, so my goal is to union them into a large region as we parse to compute spans which track the entire source fragment. i.e if I have two tokens like `def` `(` the bigger span will be `tok1->span.Merge(tok2->span)` we can do this multiple times to compute the greatest spanning region for the outer most node.
maybe more clearly a parent node's span will be roughly `MergeAll(children.map(|x| { return x.span })`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r464201530
##########
File path: src/parser/meta_ref.cc
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file src/parser/meta_ref.cc
+ * \brief An operator which allows forward referencing a yet-to-be parsed meta table reference.
+ */
+
+#include "./meta_ref.h"
+
+#include <topi/elemwise.h>
+#include <tvm/relay/expr_functor.h>
+#include <tvm/relay/op.h>
+#include <tvm/relay/op_attr_types.h>
+#include <tvm/relay/transform.h>
+
+namespace tvm {
+namespace parser {
+
+using tvm::relay::transform::CreateFunctionPass;
+using tvm::transform::PassContext;
+
+TVM_REGISTER_NODE_TYPE(MetaRefAttrs);
+
+bool MetaRefRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ LOG(FATAL) << "need to expand before type checking";
+ return true;
+}
+
+RELAY_REGISTER_OP("parser.MetaRef")
+ .describe(R"code(A reference into the meta table.)code" TVM_ADD_FILELINE)
+ .set_attrs_type<MetaRefAttrs>()
+ .set_num_inputs(0)
+ .set_support_level(10)
+ .add_type_rel("MetaRef", MetaRefRel)
+ .set_attr<TOpIsStateful>("TOpIsStateful", false)
+ .set_attr<TNonComputational>("TNonComputational", true);
+
+Expr MetaRef(std::string type_key, uint64_t node_index) {
+ static const Op& op = Op::Get("parser.MetaRef");
+ auto attrs = make_object<MetaRefAttrs>();
+ attrs->node_type_key = tvm::String(type_key);
+ attrs->node_index = node_index;
+ return Call(op, {}, Attrs(attrs), {});
+}
+
+// class MetaRefAttrExpander : AttrFunctor<ObjectRef(const ObjectRef& n)> {
+// ObjectRef VisitAttrDefault_(const Object* node) final {
+
+// }
+// }
+
+struct MetaRefExpander : public ExprMutator {
+ MetaTable table;
+
+ explicit MetaRefExpander(const MetaTable& table) : table(table) {}
+
+ Expr VisitExpr_(const CallNode* call) final {
+ if (auto op_node = call->op.as<OpNode>()) {
+ if (op_node->name == "parser.MetaRef") {
+ auto meta_attrs = call->attrs.as<MetaRefAttrs>();
+ CHECK(meta_attrs) << "an internal error has occurred";
+ auto nodes = table.at(meta_attrs->node_type_key);
+ CHECK_LT(meta_attrs->node_index, nodes.size());
+ return Downcast<Expr>(nodes[meta_attrs->node_index]);
+ }
+ }
+
+ return ExprMutator::VisitExpr_(call);
+ }
+};
+
+Function ExpandMetaRefs(const MetaTable& meta_table, const relay::Function& func) {
+ MetaRefExpander expander(meta_table);
+ return Downcast<Function>(expander.VisitExpr(func));
+}
+
+IRModule ExpandMetaRefs(const MetaTable& meta_table, const IRModule& mod) {
+ auto pass = CreateFunctionPass([&](Function func, IRModule module,
+ PassContext ctx) { return ExpandMetaRefs(meta_table, func); },
+ 1337, "ExpandMetaRefs", {});
Review comment:
Why the opt_level is 1337? Do we just want to set it with a large number?
##########
File path: src/printer/text_printer.h
##########
@@ -355,14 +355,19 @@ namespace tvm {
class TextPrinter {
public:
explicit TextPrinter(bool show_meta_data,
- const runtime::TypedPackedFunc<std::string(ObjectRef)>& annotate)
+ const runtime::TypedPackedFunc<std::string(ObjectRef)>& annotate,
+ bool show_warning = true)
: show_meta_data_(show_meta_data),
+ show_warning_(show_warning),
annotate_(annotate),
relay_text_printer_(show_meta_data, &meta_, annotate),
tir_text_printer_(show_meta_data, &meta_) {}
/*! \brief whether show meta data */
bool show_meta_data_;
+ /*! \brief whether show meta data */
Review comment:
```suggestion
/*! \brief whether show warning */
```
##########
File path: src/parser/parser.cc
##########
@@ -542,39 +501,60 @@ class Parser {
*/
template <typename T>
Array<T> ParseSequence(TokenType start, TokenType sep, TokenType stop, std::function<T()> parse,
- std::function<void()> before_stop = nullptr) {
+ std::function<bool()> before_stop = nullptr) {
+ DLOG(INFO) << "Parser::ParseSequence: start=" << start << "sep=" << sep << "stop=" << stop;
Match(start);
+
+ // This is for the empty arguments list case, if we have <start> <leftovers> <stop> token stream
+ // we must parse leftovers, then match a stop token.
+ if (before_stop) {
+ auto did_parse = before_stop();
+ if (did_parse) {
+ Match(stop);
+ return {};
+ }
+ }
+
+ // This is the case in which we find an empty arguments lists and no leftovers.
if (WhenMatch(stop)) {
return Array<T>();
} else {
auto data = parse();
Array<T> elements = {data};
- // parse '(' expr ')'
+ // parse '(' expr ','? ')'
// if we are at the end invoke leftover parser
- if (Peek()->token_type == stop && before_stop) {
- before_stop();
- }
+ // if (Peek()->token_type == sep && before_stop) {
+ // before_stop();
+ // }
Review comment:
clean up
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r465347178
##########
File path: include/tvm/relay/adt.h
##########
@@ -309,8 +309,9 @@ class Match : public Expr {
* \param data the input being deconstructed.
* \param clauses The clauses for matching.
* \param complete Indicate if this match is complete.
+ * \param span The span of the expression.
*/
- TVM_DLL Match(Expr data, tvm::Array<Clause> clauses, bool complete = true);
+ TVM_DLL Match(Expr data, tvm::Array<Clause> clauses, bool complete = true, Span span = Span());
Review comment:
I actually don't want spans to be optional if possible, but currently we need to migrate to setting them everywhere.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#issuecomment-667391205
This PR is now ready for full review, if possible can people please chime in about meta-table and related decisions. I know there are still some incomplete aspects but this is already more robust then the previous parser and I would like to incrementalize instead of sending many 4kloc PRs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463858618
##########
File path: src/parser/meta_ref.h
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file meta_ref.h
+ * \brief A reference into the metadata section of the Relay text format.
+ */
+
+#ifndef TVM_PARSER_META_REF_H_
+#define TVM_PARSER_META_REF_H_
+
+#include <tvm/ir/attrs.h>
+#include <tvm/relay/expr.h>
+#include <tvm/relay/function.h>
+
+#include <string>
+
+namespace tvm {
+namespace parser {
+
+using namespace relay;
+
+using MetaTable = Map<String, Array<ObjectRef>>;
+
+/*!
+ * \brief Options for allocating storage.
+ */
+struct MetaRefAttrs : public tvm::AttrsNode<MetaRefAttrs> {
+ tvm::String node_type_key;
+ uint64_t node_index;
+
+ TVM_DECLARE_ATTRS(MetaRefAttrs, "relay.attrs.MetaRefAttrs") {
+ TVM_ATTR_FIELD(node_type_key)
+ .describe("The type_key representing the type of the node referenced.");
+ TVM_ATTR_FIELD(node_index).describe("The index into the type specific node array.");
+ }
+};
+
+/*! \brief A reference to a "meta-expression".
+ *
+ * In the text format we allow referencing metadata which
+ * uses a compact serialization that proceeds the main
+ * program body.
+ *
+ * We can reference this table using an expression of
+ * the form `meta[Type][index]`.
+ *
+ * We must later resolve these references to actual in-memory
+ * AST nodes but this requires first parsing the full program
+ * then expanding these temporary AST nodes into their corresponding
+ * nodes.
+ *
+ * For example the nth large constant will be pretty-printed as meta[relay.Constant][n]
+ * with its compact binary serialization residing in the metadata section at the end
+ * of the program.
+ *
+ * \param type_key The type key of the object in the meta section.
+ * \param kind The index into that subfield.
+ * \returns The meta table reference.
+ */
+Expr MetaRef(std::string type_key, uint64_t node_index);
+
+relay::Function ExpandMetaRefs(const MetaTable& meta_table, const relay::Function& mod);
Review comment:
yeah I will do this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463857316
##########
File path: src/ir/span.cc
##########
@@ -61,18 +61,29 @@ TVM_REGISTER_NODE_TYPE(SourceNameNode)
return static_cast<const SourceNameNode*>(n)->name;
});
-Span::Span(SourceName source, int lineno, int col_offset) {
+Span::Span(SourceName source, int line, int column, int end_line, int end_column) {
auto n = make_object<SpanNode>();
n->source = std::move(source);
- n->line = lineno;
- n->column = col_offset;
+ n->line = line;
+ n->column = column;
+ n->end_line = end_line;
+ n->end_column = end_column;
data_ = std::move(n);
}
+Span Span::Merge(const Span& other) {
Review comment:
I converted the tokenizer to track spans as well and removed any use of raw column and/or row. Now the idea is my head is most tokens will start off as a single character or set of characters i.e (1, 1, 1, 2) or something similar to this. The tokens are consecutive, so my goal is to union them into a larger region as we parse to compute spans which track the entire source fragment. i.e if I have two tokens like `def` `(` the bigger span will be `tok1->span.Merge(tok2->span)` we can do this multiple times to compute the greatest spanning region for the outer most node.
maybe more clearly a parent node's span will be roughly `MergeAll(children.map(|x| { return x.span })`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch merged pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch merged pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r461947492
##########
File path: src/parser/meta_ref.cc
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file src/parser/meta_ref.cc
+ * \brief An operator which allows forward referencing a yet-to-be parsed meta table reference.
+ */
+
+#include <tvm/relay/op.h>
+#include <tvm/relay/expr_functor.h>
+#include <tvm/relay/transform.h>
+#include <tvm/relay/op_attr_types.h>
+#include <topi/elemwise.h>
+
+#include "./meta_ref.h"
+
+namespace tvm {
+namespace parser {
+
+using tvm::transform::PassContext;
+using tvm::relay::transform::CreateFunctionPass;
+
+TVM_REGISTER_NODE_TYPE(MetaRefAttrs);
+
+bool MetaRefRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
+ const TypeReporter& reporter) {
+ LOG(FATAL) << "need to expand before type checking";
+ return true;
+}
+
+RELAY_REGISTER_OP("parser.MetaRef")
+ .describe(R"code(A reference into the meta table.)code" TVM_ADD_FILELINE)
+ .set_attrs_type<MetaRefAttrs>()
+ .set_num_inputs(0)
+ .set_support_level(10)
+ .add_type_rel("MetaRef", MetaRefRel)
+ .set_attr<TOpIsStateful>("TOpIsStateful", false)
+ .set_attr<TNonComputational>("TNonComputational", true);
+
+Expr MetaRef(std::string type_key, uint64_t node_index) {
+ static const Op& op = Op::Get("parser.MetaRef");
+ auto attrs = make_object<MetaRefAttrs>();
+ attrs->node_type_key = tvm::String(type_key);
+ attrs->node_index = node_index;
+ return Call(op, {}, Attrs(attrs), {});
+}
+
+
+// class MetaRefAttrExpander : AttrFunctor<ObjectRef(const ObjectRef& n)> {
Review comment:
It is still draft, I will do the final clean up once the design is finalized.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463430572
##########
File path: src/parser/parser.cc
##########
@@ -542,39 +496,60 @@ class Parser {
*/
template <typename T>
Array<T> ParseSequence(TokenType start, TokenType sep, TokenType stop, std::function<T()> parse,
- std::function<void()> before_stop = nullptr) {
+ std::function<bool()> before_stop = nullptr) {
+ DLOG(INFO) << "Parser::ParseSequence: start=" << start << "sep=" << sep << "stop=" << stop;
Review comment:
Thanks! I am also in favor of more log info in TVM(more precisely operational logs stage wise). But i had the notion that we will use the new Diagnostic framework even for INFO as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#issuecomment-671818745
@tqchen this should be gtg, looks like I got passed the last few CPU tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r462735811
##########
File path: tests/python/relay/test_ir_parser2.py
##########
@@ -1,891 +0,0 @@
-# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
This was a duplicated file that was modified to work on new parser. I deleted the original parsers tests and replaced it with the one that has been ported to work on new parser.
##########
File path: tests/python/relay/test_ir_parser.py
##########
@@ -868,48 +883,60 @@ def test_extern_adt_defn():
extern_def = relay.TypeData(extern_var, [typ_var], [])
mod[extern_var] = extern_def
- assert_parses_as(
+ assert_parse_module_as(
"""
extern type T[A]
""",
mod
)
+
+@pytest.mark.skip("not yet tested on parser 2.0")
def test_import_grad():
mod = tvm.IRModule()
mod.import_from_std("gradient.rly")
+# hiearchy id, i.e parse nn.conv2d
+# do with multiple levels
+#
+# call attributes not correctly parsing
+# convert error from attribute construction to real error message
+# lexing issue with projection of graph variables
+
+# def test_hierarchical_identifiers():
+# assert False
+
+def test_resnet():
+ mod, params = relay.testing.resnet.get_workload()
+ text = str(mod.astext())
+ parsed_mod = parse_module(text)
+ tvm.ir.assert_structural_equal(mod, parsed_mod)
+
+def inline_params(mod, params):
+ main_fn = mod["main"]
+ str_to_var = {}
+ for param in main_fn.params:
+ str_to_var[param.name_hint] = param
+
+ bind_map = {}
+ for param in params:
+ bind_map[str_to_var[param]] = relay.const(params[param])
+
+ body = relay.bind(main_fn.body, bind_map)
+ main_fn = relay.Function(relay.analysis.free_vars(body), body)
+ mod["main_fn"] = main_fn
+ return mod
+
+def test_resnet_inlined_params():
+ mod, params = relay.testing.resnet.get_workload()
+ print("here")
+ mod = inline_params(mod, params)
+ print("here")
+ text = str(mod.astext())
+ print("here")
+ parsed_mod = parse_module(text)
+ print("here")
+ tvm.ir.assert_structural_equal(mod, parsed_mod)
+ print("here")
+
if __name__ == "__main__":
- test_graph()
Review comment:
See my above comment to Marisa, but in my last PR I left the existing parser in place with all tests in `tests/python/relay/test_ir_parser.py` and the new parser in `tests/python/relay/test_ir_parser2.py`. In this PR I removed the original parser completely and renamed the 2.0 tests.
##########
File path: include/tvm/parser/source_map.h
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.
+ */
+
+#ifndef TVM_PARSER_SOURCE_MAP_H_
+#define TVM_PARSER_SOURCE_MAP_H_
+/*!
+ * \file source_map.h
+ * \brief A map from source names to source code.
+ */
+#include <tvm/runtime/packed_func.h>
+#include <tvm/runtime/registry.h>
+#include <tvm/ir/span.h>
+
+#include <fstream>
+#include <string>
+
+namespace tvm {
+namespace parser {
+
+
+/*! \brief A program source in any language.
+ *
+ * Could represent the source from an ML framework or the internal
+ * source of a TVM program.
+ */
+struct Source {
+ /*! \brief The raw source. */
+ std::string source;
+ /*! \brief A mapping of line breaks into the raw source. */
+ std::vector<std::pair<int, int>> line_map;
+
+ /*! \brief An empty source. */
+ Source() : source(), line_map() {}
+
+ /*! \brief Construct a source from a string. */
+ TVM_DLL explicit Source(const std::string& source);
+
+ TVM_DLL Source(const Source& source) : source(source.source), line_map(source.line_map) {}
+
+ /*! \brief Generate an error message at a specific line and column with the
+ * annotated message.
+ *
+ * The error is written directly to the `out` std::ostream.
+ *
+ * \param out The output ostream.
+ * \param line The line at which to report a diagnostic.
+ * \param line The column at which to report a diagnostic.
+ * \param msg The message to attach.
+ */
+ TVM_DLL void ReportAt(std::ostream& out, int line, int column, const std::string& msg) const;
Review comment:
I plan on doing this in follow up work. I have a tracking issue here https://github.com/apache/incubator-tvm/issues/6153. My goal was to first rewrite the parser, land initial machinery, replace the existing one with near feature parity (I believe 2.0 is better then the existing one) and then improve as I try to flow the new diagnostics through the entire compiler pipeline. I added a bullet to the tracking list about defining a custom diagnostic renderer which will control how the messages are generated.
##########
File path: python/tvm/parser/__init__.py
##########
@@ -24,4 +24,5 @@ def parse_expr(source):
return _ffi_api.ParseExpr("string", source)
def fromtext(source, source_name="from_string"):
+ # TODO(@tqchen): currently we have to invoke `str` which dramatically reduces performance.
Review comment:
The C++ code was expecting a std::string, I changed it to tvm::String which fixes the overload but still incurs a copy. Copying can be very expensive here, for example if we have a model including the parameters it can be 100s of megabytes.
##########
File path: python/tvm/parser/__init__.py
##########
@@ -24,4 +24,5 @@ def parse_expr(source):
return _ffi_api.ParseExpr("string", source)
def fromtext(source, source_name="from_string"):
+ # TODO(@tqchen): currently we have to invoke `str` which dramatically reduces performance.
Review comment:
The C++ code was expecting a std::string, I changed it to tvm::String which fixes the overload but still incurs a copy. Copying can be very expensive here, for example if we have a model including the parameters it can be 100s of megabytes. Its slightly faster without conversion, but would be even faster if we didn't need to copy.
##########
File path: src/parser/diagnostic.h
##########
@@ -138,8 +62,73 @@ struct Diagnostic {
/*! \brief The diagnostic message. */
std::string message;
+ /*! \brief A diagnostic for a single character token. */
Diagnostic(int line, int column, const std::string& message)
- : level(DiagnosticLevel::Error), span(SourceName(), line, column), message(message) {}
+ : level(DiagnosticLevel::Error), span(SourceName(), line, column, line, column + 1), message(message) {}
+
+ Diagnostic(DiagnosticLevel level, Span span, const std::string& message)
+ : level(level), span(span), message(message) {}
+};
+
+/*!
+ * \brief A wrapper around std::stringstream to build a diagnostic.
+ *
+ * \code
+ *
+ * void ReportError(const Error& err);
+ *
+ * void Test(int number) {
+ * // Use error reporter to construct an error.
+ * ReportError(ErrorBuilder() << "This is an error number=" << number);
+ * }
+ *
+ * \endcode
+ */
+struct DiagnosticBuilder {
+ public:
+ /*! \brief The level. */
+ DiagnosticLevel level;
+
+ /*! \brief The source name. */
+ SourceName source_name;
+
+ /*! \brief The span of the diagnostic. */
+ Span span;
+
+ /*! \brief The line number. */
+ int line;
Review comment:
I am part way through refactoring, originally this didn't have the Span and it now does so we can delete the line/column fields.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r463856185
##########
File path: include/tvm/parser/source_map.h
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.
+ */
+
+#ifndef TVM_PARSER_SOURCE_MAP_H_
+#define TVM_PARSER_SOURCE_MAP_H_
+/*!
+ * \file source_map.h
+ * \brief A map from source names to source code.
+ */
+#include <tvm/runtime/packed_func.h>
+#include <tvm/runtime/registry.h>
+#include <tvm/ir/span.h>
+
+#include <fstream>
+#include <string>
+
+namespace tvm {
+namespace parser {
+
+
+/*! \brief A program source in any language.
+ *
+ * Could represent the source from an ML framework or the internal
+ * source of a TVM program.
+ */
+struct Source {
+ /*! \brief The raw source. */
+ std::string source;
+ /*! \brief A mapping of line breaks into the raw source. */
+ std::vector<std::pair<int, int>> line_map;
+
+ /*! \brief An empty source. */
+ Source() : source(), line_map() {}
+
+ /*! \brief Construct a source from a string. */
+ TVM_DLL explicit Source(const std::string& source);
+
+ TVM_DLL Source(const Source& source) : source(source.source), line_map(source.line_map) {}
+
+ /*! \brief Generate an error message at a specific line and column with the
+ * annotated message.
+ *
+ * The error is written directly to the `out` std::ostream.
+ *
+ * \param out The output ostream.
+ * \param line The line at which to report a diagnostic.
+ * \param line The column at which to report a diagnostic.
+ * \param msg The message to attach.
+ */
+ TVM_DLL void ReportAt(std::ostream& out, int line, int column, const std::string& msg) const;
+};
+
+/*!
+ * \brief A mapping from a unique source name to source fragment.
+ */
+class SourceMap;
Review comment:
I wanted to split out all the source_map management into a logic header to me its distinct from the spans, if anything I would put span's into source_map.h as they are really just indicies into a source file. I was also attempting to keep the less stable interface in the parser headers for now as parser/diagnostics will evolve quickly. Once we are happy with the interface we can stabilize by lifting to the top-level include directory.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6162: [Parser] Parser 2.0 part 2
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6162:
URL: https://github.com/apache/incubator-tvm/pull/6162#discussion_r465336293
##########
File path: include/tvm/ir/span.h
##########
@@ -85,16 +85,26 @@ class SpanNode : public Object {
int line;
Review comment:
This is kind of painful to refactor due some of the Python code. I still need to update the Python bindings how about I add to tracking issue and we can do in follow up work?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org