jroesch commented on a change in pull request #6274: URL: https://github.com/apache/incubator-tvm/pull/6274#discussion_r493083336
########## File path: include/tvm/ir/diagnostic.h ########## @@ -0,0 +1,254 @@ +/* + * 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 diagnostic.h + * \brief A new diagnostic interface for TVM error reporting. + * + * A prototype of the new diagnostic reporting interface for TVM. + * + * Eventually we hope to promote this file to the top-level and + * replace the existing errors.h. + */ + +#ifndef TVM_IR_DIAGNOSTIC_H_ +#define TVM_IR_DIAGNOSTIC_H_ + +#include <tvm/ir/span.h> +#include <tvm/ir/module.h> +#include <tvm/parser/source_map.h> +#include <tvm/runtime/container.h> +#include <tvm/runtime/object.h> +#include <tvm/support/logging.h> + +#include <fstream> +#include <string> +#include <utility> +#include <vector> + +namespace tvm { + +using tvm::parser::SourceMap; + +static const char* kTVM_INTERNAL_ERROR_MESSAGE = "An internal invariant was violated during the execution of TVM" \ + "please read TVM's error reporting guidelines at discuss.tvm.ai/thread"; + +static const char* kINDENT = " "; + +#define ICHECK_BINARY_OP(name, op, x, y) \ + if (dmlc::LogCheckError _check_err = dmlc::LogCheck##name(x, y)) \ + dmlc::LogMessageFatal(__FILE__, __LINE__).stream() \ + << kTVM_INTERNAL_ERROR_MESSAGE << std::endl \ + << kIndent << "Check failed: " << #x " " #op " " #y << *(_check_err.str) << ": " + +#define ICHECK(x) \ + if (!(x)) \ + dmlc::LogMessageFatal(__FILE__, __LINE__).stream() \ + << kTVM_INTERNAL_ERROR_MESSAGE \ + << kINDENT << "Check failed: " #x << ": " + +#define ICHECK_LT(x, y) ICHECK_BINARY_OP(_LT, <, x, y) +#define ICHECK_GT(x, y) ICHECK_BINARY_OP(_GT, >, x, y) +#define ICHECK_LE(x, y) ICHECK_BINARY_OP(_LE, <=, x, y) +#define ICHECK_GE(x, y) ICHECK_BINARY_OP(_GE, >=, x, y) +#define ICHECK_EQ(x, y) ICHECK_BINARY_OP(_EQ, ==, x, y) +#define ICHECK_NE(x, y) ICHECK_BINARY_OP(_NE, !=, x, y) +#define ICHECK_NOTNULL(x) \ + ((x) == NULL ? dmlc::LogMessageFatal(__FILE__, __LINE__).stream() \ + << kTVM_INTERNAL_ERROR_MESSAGE \ + << kINDENT << "Check not null: " #x << ' ', (x) : (x)) // NOLINT(*) + +/*! \brief The diagnostic level, controls the printing of the message. */ +enum class DiagnosticLevel { + kBug, + kError, + kWarning, + kNote, + kHelp, +}; Review comment: Yeah this probably useful for Python I generally like to avoid using int enums. ########## File path: include/tvm/ir/module.h ########## @@ -280,12 +284,14 @@ class IRModule : public ObjectRef { * \param functions Functions in the module. * \param type_definitions Type definitions in the module. * \param import_set Set of imported files in the module + * \param map The module source map. */ TVM_DLL explicit IRModule(Map<GlobalVar, BaseFunc> functions, Map<GlobalTypeVar, TypeData> type_definitions = {}, - std::unordered_set<String> import_set = {}); + std::unordered_set<String> import_set = {}, parser::SourceMap map = {}); + /*! \brief default constructor */ - IRModule() : IRModule(Map<GlobalVar, BaseFunc>()) {} Review comment: Add this back, we should make IRModule non-null in follow up work. ########## File path: python/tvm/ir/module.py ########## @@ -72,9 +72,9 @@ def __setitem__(self, var, val): val: Union[Function, Type] The value. """ - return self._add(var, val) + return self._add(var, val, True) - def _add(self, var, val, update=False): + def _add(self, var, val, update=True): Review comment: Remove update/add in future PR. ########## File path: python/tvm/parser/__init__.py ########## @@ -28,3 +29,11 @@ def parse_expr(source): def fromtext(source, source_name="from_string"): return parse(source, source_name) + + +def SpanCheck(): + return _ffi_api.SpanCheck() + + +def AnnotateSpans(): Review comment: Move this pass to `transform`. ########## File path: python/tvm/relay/backend/interpreter.py ########## @@ -213,13 +213,15 @@ def optimize(self): """ seq = tvm.transform.Sequential( [ + # tvm.parser.AnnotateSpans(), Review comment: Should we provide a flag for spans here? ########## File path: python/tvm/relay/prelude.py ########## @@ -1512,3 +1552,6 @@ def load_prelude(self): ]: tensor_array_ops = TensorArrayOps(self, dtype) tensor_array_ops.register() + + # Renamer doesn't properly deal with constructors, etc Review comment: I don't think this is true anymore. ########## File path: python/tvm/relay/std/prelude.rly ########## @@ -193,7 +194,7 @@ def @zip[A, B](%xs: List[A], %ys: List[B]) -> List[(A, B)] { * Reverses a list. */ def @rev[A](%xs: List[A]) -> List[A] { - @foldl(@flip(Cons), Nil, %xs) + @foldl(@flip(fn (%h: A, %t: List[A]) { Cons(%h, %t) }), Nil, %xs) Review comment: The parser must eta expand constructors, should file this as an issue. ########## File path: src/printer/relay_text_printer.cc ########## @@ -522,10 +522,11 @@ Doc RelayTextPrinter::VisitExpr_(const MatchNode* op) { Doc clause_doc; clause_doc << PrintPattern(clause->lhs, false) << " => "; Doc rhs_doc = PrintScope(clause->rhs); - if (clause->rhs.as<LetNode>()) { - // only add braces if there are multiple lines on the rhs - rhs_doc = Doc::Brace("{", rhs_doc, "}"); - } + // TODO(@jroesch): This is unsound right now, and we need to revist it. Review comment: We need to open an issue, some of the printing doesn't fully match the parser. ########## File path: src/parser/parser.cc ########## @@ -661,15 +754,21 @@ class Parser { Consume(TokenType::kDefn); auto global_tok = Match(TokenType::kGlobal); auto global_name = global_tok.ToString(); - auto global = GlobalVar(global_name); - try { - global_names.Add(global_name, global); - } catch (const DuplicateKeyError& e) { - this->diag_ctx->Emit(Diagnostic::Error(global_tok->span) << "a function with the name " - << "`@" << global_name << "` " - << "was previously defined"); - } - auto func = ParseFunctionDef(); + auto global = AddOrGet(&global_names, global_name); + auto func = WithSpan<relay::Function>([&]() { + // try { Review comment: Fix the error handling around this, was blanket catching exceptions earlier which is an issue. ########## File path: src/parser/parser.cc ########## @@ -699,15 +798,15 @@ class Parser { // Parse the type's identifier. auto type_tok = Match(TokenType::kIdentifier); auto type_id = type_tok.ToString(); - auto type_global = tvm::GlobalTypeVar(type_id, TypeKind::kAdtHandle); + auto type_global = AddOrGet(&type_names, type_id, TypeKind::kAdtHandle); - try { - type_names.Add(type_id, type_global); - } catch (const DuplicateKeyError& e) { - this->diag_ctx->Emit(Diagnostic::Error(type_tok->span) << "a type definition with the name " - << "`" << type_id << "` " - << "was previously defined"); - } + // try { Review comment: This is now lazily resolved, I think I should just purge this error handling. ########## File path: src/parser/parser.cc ########## @@ -1527,29 +1699,50 @@ class Parser { // Parses a user defined ADT or type variable. Type ParseNonPrimitiveType(const Token& tok) { - auto name = tok.ToString(); - Type head_type; - auto global_type = type_names.Get(name); + return WithSpan<Type>([&]() { + auto name = tok.ToString(); + Type head_type = LookupTypeVar(tok); - if (!global_type) { - head_type = LookupTypeVar(tok); - } else { - head_type = global_type.value(); - } + if (!head_type.defined()) { + // head_type = type_names.Get(name); + head_type = AddOrGet(&type_names, name, TypeKind::kAdtHandle); + } - CHECK(head_type.defined()) << "internal error: head type must be defined"; + // if (!var.defined()) { Review comment: We now lazily resolve names in order to defer handling, we will merge checking this into the well-formed check in follow up work to clean up the handling of 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: [email protected]
