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/10/15 20:29:14 UTC

[GitHub] [incubator-tvm] rkimball opened a new pull request #6692: Refactor diagnostic to avoid circular dependencies

rkimball opened a new pull request #6692:
URL: https://github.com/apache/incubator-tvm/pull/6692


   There were circular dependencies in object.h which made use of ICHECK in Object problematic.


----------------------------------------------------------------
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 #6692: Refactor diagnostic to avoid circular dependencies

Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6692:
URL: https://github.com/apache/incubator-tvm/pull/6692#discussion_r505858000



##########
File path: include/tvm/ir/diagnostic_context.h
##########
@@ -0,0 +1,222 @@
+/*
+ * 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_CONTEXT_H_
+#define TVM_IR_DIAGNOSTIC_CONTEXT_H_
+
+#include <tvm/ir/module.h>
+#include <tvm/parser/source_map.h>
+
+#include <sstream>
+#include <string>
+
+namespace tvm {
+
+using tvm::parser::SourceMap;
+using tvm::runtime::TypedPackedFunc;
+
+extern const char* kTVM_INTERNAL_ERROR_MESSAGE;
+
+class DiagnosticBuilder;
+
+/*! \brief A compiler diagnostic. */
+class Diagnostic;
+
+/*! \brief A compiler diagnostic message. */
+class DiagnosticNode : public Object {
+ public:
+  /*! \brief The level. */

Review comment:
       We should improve this comment. 




----------------------------------------------------------------
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 #6692: Refactor diagnostic to avoid circular dependencies

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6692:
URL: https://github.com/apache/incubator-tvm/pull/6692#issuecomment-709642068


   In this particular case, ICHECK perhaps should not end up in ir/diagnostic, but instead in `support/logging.h`, moving the marco defs there would be the simplest solution


----------------------------------------------------------------
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 #6692: Refactor diagnostic to avoid circular dependencies

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6692:
URL: https://github.com/apache/incubator-tvm/pull/6692#issuecomment-711099531


   Thanks @rkimball @jroesch 


----------------------------------------------------------------
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 a change in pull request #6692: Refactor diagnostic to avoid circular dependencies

Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6692:
URL: https://github.com/apache/incubator-tvm/pull/6692#discussion_r506732411



##########
File path: include/tvm/ir/diagnostic_context.h
##########
@@ -18,71 +18,25 @@
  */
 
 /*!
- * \file diagnostic.h
+ * \file diagnostic_context.h

Review comment:
       now that we have removed diagnostic.h I think it is better to just rename back to diagnostic.h for simplicity




----------------------------------------------------------------
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 a change in pull request #6692: Refactor diagnostic to avoid circular dependencies

Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6692:
URL: https://github.com/apache/incubator-tvm/pull/6692#discussion_r506560441



##########
File path: include/tvm/support/diagnostic_context.h
##########
@@ -27,62 +27,20 @@
  * replace the existing errors.h.
  */
 
-#ifndef TVM_IR_DIAGNOSTIC_H_
-#define TVM_IR_DIAGNOSTIC_H_
+#ifndef TVM_IR_DIAGNOSTIC_CONTEXT_H_
+#define TVM_IR_DIAGNOSTIC_CONTEXT_H_
 
 #include <tvm/ir/module.h>
-#include <tvm/ir/span.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 <sstream>
 #include <string>
-#include <utility>
-#include <vector>
 
 namespace tvm {
 
 using tvm::parser::SourceMap;
 using tvm::runtime::TypedPackedFunc;
 
-extern const char* kTVM_INTERNAL_ERROR_MESSAGE;
-
-#define ICHECK_INDENT "  "
-
-#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                  \
-      << ICHECK_INDENT << "Check failed: " << #x " " #op " " #y << *(_check_err.str) << ": "
-
-#define ICHECK(x)                                    \
-  if (!(x))                                          \
-  dmlc::LogMessageFatal(__FILE__, __LINE__).stream() \
-      << kTVM_INTERNAL_ERROR_MESSAGE << ICHECK_INDENT << "Check failed: " #x << " == false: "
-
-#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) == nullptr ? dmlc::LogMessageFatal(__FILE__, __LINE__).stream()                      \
-                        << kTVM_INTERNAL_ERROR_MESSAGE << __INDENT << "Check not null: " #x \
-                        << ' ',                                                             \
-   (x) : (x))  // NOLINT(*)
-
-/*! \brief The diagnostic level, controls the printing of the message. */
-enum class DiagnosticLevel : int {
-  kBug = 10,
-  kError = 20,
-  kWarning = 30,
-  kNote = 40,
-  kHelp = 50,
-};
-

Review comment:
       diagnostic context should not be part of support, because right now it brings large amount of dep related to IR. keep it as in include/tvm/ir/diagnostic.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 merged pull request #6692: Refactor diagnostic to avoid circular dependencies

Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #6692:
URL: https://github.com/apache/incubator-tvm/pull/6692


   


----------------------------------------------------------------
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 #6692: Refactor diagnostic to avoid circular dependencies

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6692:
URL: https://github.com/apache/incubator-tvm/pull/6692#issuecomment-710126019


   As a minimum set of changes to the PR. i would recommend keep the files as they are, and just move the ICHECK defs into `support/logging.h`
   
   the additional diagnostics can still be part of the tvm/ir/diagnostic.h, and we do not need the additional context suffix that might cause confusion. 
   
   


----------------------------------------------------------------
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 a change in pull request #6692: Refactor diagnostic to avoid circular dependencies

Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6692:
URL: https://github.com/apache/incubator-tvm/pull/6692#discussion_r506559623



##########
File path: include/tvm/support/diagnostic.h
##########
@@ -0,0 +1,74 @@
+/*
+ * 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 <dmlc/logging.h>
+
+namespace tvm {
+
+extern const char* kTVM_INTERNAL_ERROR_MESSAGE;

Review comment:
       Merge this file content with tvm/support/logging.h as previous content are already there

##########
File path: include/tvm/support/diagnostic_context.h
##########
@@ -27,62 +27,20 @@
  * replace the existing errors.h.
  */
 
-#ifndef TVM_IR_DIAGNOSTIC_H_
-#define TVM_IR_DIAGNOSTIC_H_
+#ifndef TVM_IR_DIAGNOSTIC_CONTEXT_H_
+#define TVM_IR_DIAGNOSTIC_CONTEXT_H_
 
 #include <tvm/ir/module.h>
-#include <tvm/ir/span.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 <sstream>
 #include <string>
-#include <utility>
-#include <vector>
 
 namespace tvm {
 
 using tvm::parser::SourceMap;
 using tvm::runtime::TypedPackedFunc;
 
-extern const char* kTVM_INTERNAL_ERROR_MESSAGE;
-
-#define ICHECK_INDENT "  "
-
-#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                  \
-      << ICHECK_INDENT << "Check failed: " << #x " " #op " " #y << *(_check_err.str) << ": "
-
-#define ICHECK(x)                                    \
-  if (!(x))                                          \
-  dmlc::LogMessageFatal(__FILE__, __LINE__).stream() \
-      << kTVM_INTERNAL_ERROR_MESSAGE << ICHECK_INDENT << "Check failed: " #x << " == false: "
-
-#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) == nullptr ? dmlc::LogMessageFatal(__FILE__, __LINE__).stream()                      \
-                        << kTVM_INTERNAL_ERROR_MESSAGE << __INDENT << "Check not null: " #x \
-                        << ' ',                                                             \
-   (x) : (x))  // NOLINT(*)
-
-/*! \brief The diagnostic level, controls the printing of the message. */
-enum class DiagnosticLevel : int {
-  kBug = 10,
-  kError = 20,
-  kWarning = 30,
-  kNote = 40,
-  kHelp = 50,
-};
-

Review comment:
       diagnostic context should not be part of support, because right now it brings large amount of dep and not part of ir. keep it as in include/tvm/ir/diagnostic.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 edited a comment on pull request #6692: Refactor diagnostic to avoid circular dependencies

Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #6692:
URL: https://github.com/apache/incubator-tvm/pull/6692#issuecomment-710126019


   Thanks @rkimball . I feel the PR can still be simplified further to avoid potential confusion of two diagnostic header files(one minimum and another built for IR). In particular we can do:
   
   - Keep all the content of ir/diagnostic.h as it is in IR, because they introduce a larger trunk of deps, it should be part of IR and tvm runtime should not depend on them.
   - Move all the ICHECK def into support/logging.h, where there is already some previous related definitions of checks, this is a minimum check macro that can be used in runtime
   
   


----------------------------------------------------------------
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 edited a comment on pull request #6692: Refactor diagnostic to avoid circular dependencies

Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #6692:
URL: https://github.com/apache/incubator-tvm/pull/6692#issuecomment-709642068


   In this particular case, ICHECK perhaps should not end up in ir/diagnostic, but instead in `support/logging.h`, moving the marco defs there would be the simplest solution. It would also avoid the dep from runtime to ir


----------------------------------------------------------------
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