junrushao1994 commented on pull request #9729: URL: https://github.com/apache/tvm/pull/9729#issuecomment-993208836
Thank you @mbs-octoml and @comaniac for providing important context on this PR! Appreciated you guys doing so when discussing architectural changes! A fundamental data structure, say an IR, for example, CFG in LLVM, is a serious promise to its end users that it will compile correctly. Changing the IR to the system means a sudden change of such a promise. In the worse case, unless all the IR passes are refactored or re-designed, it’s not easy to thoroughly implement the new promise on top of IR change. 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 was 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. As the basic unit of TVM unity and unified IR, the promise of IRModule includes: - With target and pass context, all passes are IRModule-to-IRModule transformation - Codegen is IRModule-to-runtime-Module transformation - Every pass handles IRModule properly without bugs - IRModule is strictly serializable - ... First-class fields, however, is a strong promise to introduce, and always we weigh a lot of factors when deliberating if something needs to be first-class supported. By introducing runtime Modules into IRModule, we are explicitly promising that all of our compiler passes could handle it, and encouraging users to assume that the Relay compilation infrastructure works perfectly with this field. This is unfortunately not true, which leads to a broken promise that we would rather avoid. For example, if we have a pass that processes GlobalVars, it will probably need to be refactored to treat this field properly to make sure our compiler isn’t broken, which is huge amount of engineering and potential source of regressions if not taken care of carefully. Another example is that we need to discuss and actually improve type inference when calling this external GlobalVar. Moreover, not all runtime Modules are serializable or printable like IRModule is, which means the serialization infrastructure will be refactored to provide special support for it (although it’s practically not serializable though for certain types of runtime Modules) Alternatively, storing runtime::Module in attributes, as Mark has pointed out, is consistent with our philosophy, where attribute is a place to store fields that passes could optionally process. For example, meta schedule uses the following collection of attributes to guide the scheduler/compiler rewriting, and it’s totally fine that compiler passes ignore the attributes: - meta_schedule.cooperative_fetch - meta_schedule.auto_tensorize - meta_schedule.tensor_core_enabled - meta_schedule.cache_type - meta_schedule.parallel - meta_schedule.vectorize - meta_schedule.unroll_explicit - meta_schedule.unroll_implicit - software_pipeline_scope - software_pipeline_order - ... This design exists in other compilers too. For example, pragmas are provided as a way to guide CUDA, OpenMP or Pluto polyhedral compiler to do rewriting, while didn’t break the original IR data structure. LLVM widely adopts this notion as well. Furthermore, gluing compilation artifacts seems like a job a linker does, instead of the main compilation process. Therefore, I would rather prefer to be consistent in terms of using attributes to store optional fields, so that there is clear boundary between user expectation and information that passes optionally process. I’m happy to discuss more! -- 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
