[TIR] Add spans to all ExprNodes

[tkonolige]

Add optional spanning information to BaseExprNode. This PR does not fill in this spanning information.

@jroesch @junrushao1994

[junrushao]

CC: @spectrometerHBH

[giuseros]

Hi @tkonolige ,
Is this related with the Synr change #6797 ? I am wondering if, from an high level, there is any other alternative than passing the span object, at construction, in the ExprNodes. If I understand correctly this is a sort of debugging utility. What if in the future we will need more debugging utilities? Wouldn't it be better to create like a DebugInfo struct that contains the span? Another alternative would be to have a debug-table with ExprNode -> DebugInfo, like a symbol table for debugging.

[tkonolige]

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

[giuseros]

@tkonolige is there an RFC (or anything similar) discussing these interface changes with an evaluation of the alternative designs?

[tkonolige]

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

[giuseros]

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 ?

[jroesch]

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

[giuseros]

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!

[giuseros]

LGTM after discussion about the interface change with @tkonolige and @jroesch

[tqchen]

Thanks @tkonolige @giuseros @jroesch