junrushao1994 edited a comment on pull request #9729: URL: https://github.com/apache/tvm/pull/9729#issuecomment-993208836
Thank you everyone for raising the PR and providing important context on this PR! I really appreciate that we come together and discuss architectural changes. These discussions makes TVM better. ## Motivation of This PR The main motivation is to bring a runtime Module onto IRModule. Runtime Module that is generated through BYOC is previously stored in the attributes of an IRModule. One motivation of this PR is to make it more explicit by moving this to a field of IRModule. Notably, both the existing attribute-based approach and proposed field-based approached could equivalently support the feature we need. To make the discussion more concise, we denote: - A1. Adding new runtime::Modules as a required field to IRModule - A2. Optionally storing BYOC runtime::Module inside IRModule's attributes ## Desired Solution The fundamental need here is that we need to store compiled artifact as part of the IRModule so the IRModule can serve a single source of truth throughout compilation. However, as Mark pointed out, the runtime::Module right now has been two overloaded properties: - P1. Used to represent compiled artifact - P2. Used to represent in-memory data structure as a collection of packed functions Runtime::Module was designed for P2 but not very clearly for P1. As a result, most of the runtime::Modules are not bidirectionally serializable. However, such properties are important for IRModule. Therefore, an ultimate solution is to introduce another object for compiled functions, i.e. Artifact. We had some discussion previously (https://discuss.tvm.apache.org/t/introduce-artifact-a-container-for-generated-code/11099) but it hasn't been pinned down yet. The main difference between runtime::Module and Artifact is: in the case of runtime::Module, serializability is a less of a concern; However, in the case of Artifact, while there are possible cases, things are still not always serializable. For example, in-memory JIT'ed artifacts. But serializability is always a first-class concern for designers of the IR itself. ## Design Discussion A fundamental data structure, say an IR, for example, CFG in LLVM, is a common ground for all the developers. Changing the IR usually needs deliberation because it affects all the developers of the compiler. An ancient example (which I believe has been fixed recently) is dead-code elimination in Relay - at the time of implementing that pass, it relied on an assumption that there were no effects in Relay, which became untrue immediately after RefRead/RefWrite were introduced. And after that IR change, the promise of this pass broke and led to overall collapse of the compiler infrastructure when fed with it the new IR nodes. Introducing this field to IRModule right now can bring a different set of expectations. More specifically, - pass writers need to consider if they have to deal with runtime::Modules collections - Additionally, it will bring up the concern of first-class fields' serializability Putting it in the attributes, on the other hand, is less a serious commitment. Of course, it's hard to jump to the ultimate solution (Artifacts). Considering the path of migration, it's better to keep the fields as optional attributes for now, so migrating to Artifact-based solution is easier. It is also a common practice in compilers. Usually a solution starts with intrinsics, attributes and pragmas, and evolves gradually into first-class fields as design matures. I believe there is good motivation for the feature change. The IR change, motivated by the same feature, could unintentionally broad impact. It's great to have to have such discussion as part of the RFC. Constructive discussion of all technical perspectives makes the design better! Thank you all for having constructive discussion! -- 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. To unsubscribe, e-mail: commits-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
