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/11/06 01:35:22 UTC
[GitHub] [incubator-tvm] tkonolige opened a new pull request #6860: [TIR] Add spans to all ExprNodes
tkonolige opened a new pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860
Add optional spanning information to BaseExprNode. This PR does not fill in this spanning information.
@jroesch @junrushao1994
----------------------------------------------------------------
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 pull request #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#issuecomment-723196876
@giuseros This is the RFC that covers all error handling: https://discuss.tvm.apache.org/t/rfc-meta-rfc-3-pronged-plan-for-improving-error-messages-in-tvm. This PR is really just a continuation of #6274.
----------------------------------------------------------------
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 #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#discussion_r518867583
##########
File path: src/ir/expr.cc
##########
@@ -55,20 +55,21 @@ PrimExpr PrimExpr::FromObject_(ObjectRef ref) {
return Downcast<PrimExpr>(ref);
}
-IntImm::IntImm(DataType dtype, int64_t value) {
- ICHECK(dtype.is_scalar()) << "ValueError: IntImm can only take scalar.";
- ICHECK(dtype.is_int() || dtype.is_uint()) << "ValueError: IntImm supports only int or uint type.";
+IntImm::IntImm(DataType dtype, int64_t value, Span span) {
+ ICHECK(dtype.is_scalar()) << "ValueError: IntImm can only take scalar, but " << dtype << " was supplied.";
+ ICHECK(dtype.is_int() || dtype.is_uint()) << "ValueError: IntImm supports only int or uint type, but " << dtype << " was supplied.";
Review comment:
Yes, I need it in `python/tvm/tir/stmt.py` to handle passing multiple optional arguments to a C++ function (in this case the Store and Load constructors).
----------------------------------------------------------------
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 #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#issuecomment-723230910
@giuseros The entire Relay AST has had spans for the entire existence of the IR, this change is a follow-on from UnifiedIR refactors where we make things more consistent.
The span design originates (or Location) style of diagnostics is the style adopted by many modern compilers including Rust, and MLIR. The reason to have spans directly on the AST is the same reason to have type information they are important fields and having them be "intrinsic" vs. "extrinsic" properties.
In my exp. working on compilers having things exist in global stateful maps which must be kept in sync introduces complexity as the global state must be passed everywhere and you have to be very careful at which time you read from the maps. For example propagating span information inside of a pass which builds new AST fragments is easy as you can directly build span information from existing spans.
If we want to attach more meta-data for diagnostics I think we should attach that information to the diagnostic objects instead of attaching them to the spans/ast nodes directly. The diagnostics correspond to a location where some information was generated and the spans are indexes into the source representation.
----------------------------------------------------------------
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] giuseros commented on pull request #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#issuecomment-723194362
@tkonolige is there an RFC (or anything similar) discussing these changes with an evaluation of the alternative designs?
----------------------------------------------------------------
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 #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860
----------------------------------------------------------------
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 pull request #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#issuecomment-722773439
CC: @spectrometerHBH
----------------------------------------------------------------
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 pull request #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#issuecomment-723180231
@giuseros This is related to #6797. The goal is to use tvmscript to provide line numbers for TIR. In the future we would like all TIR nodes to have span information. This PR is also related to #6274 in which spanning information was added to all relay nodes.
You'll have to ask @jroesch about the why we are using Spans vs a more complex DebugInfo struct. I know in the future we may want more complex debugging info.
----------------------------------------------------------------
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 #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#issuecomment-725044052
Thanks @tkonolige @giuseros @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] tkonolige commented on a change in pull request #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#discussion_r518868048
##########
File path: src/ir/expr.cc
##########
@@ -55,20 +55,21 @@ PrimExpr PrimExpr::FromObject_(ObjectRef ref) {
return Downcast<PrimExpr>(ref);
}
-IntImm::IntImm(DataType dtype, int64_t value) {
- ICHECK(dtype.is_scalar()) << "ValueError: IntImm can only take scalar.";
- ICHECK(dtype.is_int() || dtype.is_uint()) << "ValueError: IntImm supports only int or uint type.";
+IntImm::IntImm(DataType dtype, int64_t value, Span span) {
+ ICHECK(dtype.is_scalar()) << "ValueError: IntImm can only take scalar, but " << dtype << " was supplied.";
+ ICHECK(dtype.is_int() || dtype.is_uint()) << "ValueError: IntImm supports only int or uint type, but " << dtype << " was supplied.";
Review comment:
I just cleaned up the error messages while I was there.
----------------------------------------------------------------
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] giuseros commented on a change in pull request #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#discussion_r518685847
##########
File path: src/ir/expr.cc
##########
@@ -55,20 +55,21 @@ PrimExpr PrimExpr::FromObject_(ObjectRef ref) {
return Downcast<PrimExpr>(ref);
}
-IntImm::IntImm(DataType dtype, int64_t value) {
- ICHECK(dtype.is_scalar()) << "ValueError: IntImm can only take scalar.";
- ICHECK(dtype.is_int() || dtype.is_uint()) << "ValueError: IntImm supports only int or uint type.";
+IntImm::IntImm(DataType dtype, int64_t value, Span span) {
+ ICHECK(dtype.is_scalar()) << "ValueError: IntImm can only take scalar, but " << dtype << " was supplied.";
+ ICHECK(dtype.is_int() || dtype.is_uint()) << "ValueError: IntImm supports only int or uint type, but " << dtype << " was supplied.";
Review comment:
Is this related with the "span" change?
##########
File path: src/tir/op/op.cc
##########
@@ -922,4 +922,8 @@ TVM_REGISTER_GLOBAL("tir._OpIfThenElse")
return if_then_else(cond, true_value, false_value);
});
+TVM_REGISTER_GLOBAL("tir.const_true").set_body_typed([](DataType t) {
+ return const_true(t.lanes());
+});
+
Review comment:
Is this related with the "span" change?
----------------------------------------------------------------
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] giuseros commented on pull request #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#issuecomment-723235173
Hi @jroesch ,
Thanks for the brilliant explanation! I still think that it would have been very nice to have this explanation on the forum (also for future reference and future changes). For this change, if we commit to not adding more to the AST, I am happy to approve!
----------------------------------------------------------------
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 #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#discussion_r518867772
##########
File path: src/tir/op/op.cc
##########
@@ -922,4 +922,8 @@ TVM_REGISTER_GLOBAL("tir._OpIfThenElse")
return if_then_else(cond, true_value, false_value);
});
+TVM_REGISTER_GLOBAL("tir.const_true").set_body_typed([](DataType t) {
+ return const_true(t.lanes());
+});
+
Review comment:
Yes, I need it in `python/tvm/tir/stmt.py` to handle passing multiple optional arguments to a C++ function (in this case the Store and Load constructors).
----------------------------------------------------------------
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] giuseros commented on pull request #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#issuecomment-723204745
Well, I meant an RFC discussing this interface change. In general, I think those interface changes should be first discussed in the discuss forum, and their implementation should then be discussed in the PR. Probably the same applies to the Relay PR you mentioned.
Anyway, I am OK in continuing the interface discussion here. I think that adding a single span parameter to all the Expr nodes risks of polluting the interface (especially if, as you said, in the future more info will be needed). Is it possible to wrap the span at least in a DebugInfo structure? What are the deltas of this 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] giuseros edited a comment on pull request #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
giuseros edited a comment on pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#issuecomment-723235173
Hi @jroesch ,
Thanks for the detailed explanation! I still think that it would have been very nice to have this explanation on the forum (also for future reference and future changes). For this change, if we commit to not adding more to the AST, I am happy to approve!
----------------------------------------------------------------
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] giuseros edited a comment on pull request #6860: [TIR] Add spans to all ExprNodes
Posted by GitBox <gi...@apache.org>.
giuseros edited a comment on pull request #6860:
URL: https://github.com/apache/incubator-tvm/pull/6860#issuecomment-723194362
@tkonolige is there an RFC (or anything similar) discussing these interface changes with an evaluation of the alternative designs?
----------------------------------------------------------------
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