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 2021/12/13 17:30:58 UTC

[GitHub] [tvm] mikepapadim opened a new pull request #9729: [CORE] Make external mods first class field in the IR module

mikepapadim opened a new pull request #9729:
URL: https://github.com/apache/tvm/pull/9729


   Co-authored-by: Lily Orth-Smith <li...@gmail.com>
   
   This PR extracts the external modules used by the TECOmpiler and various places in the AOT flow and makes them first class fields in the IRModule.
   
   @mbs-octoml @electriclilies 


-- 
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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] comaniac commented on a change in pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

comaniac commented on a change in pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#discussion_r768008092



##########
File path: include/tvm/ir/module.h
##########
@@ -57,6 +57,8 @@ class IRModuleNode : public Object {
   Map<GlobalVar, BaseFunc> functions;
   /*! \brief A map from global type vars to ADT type data. */
   Map<GlobalTypeVar, TypeData> type_definitions;
+  /*! \brief The external module containing the external functions used by this module. */
+  Array<runtime::Module> external_mods;

Review comment:
       This is not a proper approach. @zhiics and I were trying to adopt the similar approach when implementing BYOC, but we gave up and embarrassed metadata module after discussing with @tqchen. In general, we should differentiate `external` modules because all modules are unified.




-- 
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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] junrushao1994 commented on a change in pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

junrushao1994 commented on a change in pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#discussion_r767980273



##########
File path: include/tvm/ir/module.h
##########
@@ -57,6 +57,8 @@ class IRModuleNode : public Object {
   Map<GlobalVar, BaseFunc> functions;
   /*! \brief A map from global type vars to ADT type data. */
   Map<GlobalTypeVar, TypeData> type_definitions;
+  /*! \brief The external module containing the external functions used by this module. */
+  Array<runtime::Module> external_mods;

Review comment:
       Not sure if it's a right way to do so by mixing IR with Runtime

##########
File path: include/tvm/ir/module.h
##########
@@ -57,6 +57,8 @@ class IRModuleNode : public Object {
   Map<GlobalVar, BaseFunc> functions;
   /*! \brief A map from global type vars to ADT type data. */
   Map<GlobalTypeVar, TypeData> type_definitions;
+  /*! \brief The external module containing the external functions used by this module. */
+  Array<runtime::Module> external_mods;

Review comment:
       Not sure if it's a right way to do so by mixing IR and 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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] junrushao1994 commented on a change in pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

junrushao1994 commented on a change in pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#discussion_r768036732



##########
File path: include/tvm/ir/module.h
##########
@@ -57,6 +57,8 @@ class IRModuleNode : public Object {
   Map<GlobalVar, BaseFunc> functions;
   /*! \brief A map from global type vars to ADT type data. */
   Map<GlobalTypeVar, TypeData> type_definitions;
+  /*! \brief The external module containing the external functions used by this module. */
+  Array<runtime::Module> external_mods;

Review comment:
       I don't think we are directly saying "no" here. To better organize our discussion:
   1. It mixes compile-time IR and the runtime thing - usually it's done by a linker instead.
   2. There exists better alternatives to use metadata module.
   
   Cody mainly wanted to introduce some history in the design rationale.
   




-- 
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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] junrushao1994 edited a comment on pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] junrushao1994 edited a comment on pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

junrushao1994 edited a comment on pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#issuecomment-992910847


   I'm writing some discussion on the technical side. (bear with me i'm a slow typer) BTW, It would be great if any IR changes could go through the RFC process.


-- 
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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mbs-octoml commented on a change in pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

mbs-octoml commented on a change in pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#discussion_r768029280



##########
File path: include/tvm/ir/module.h
##########
@@ -57,6 +57,8 @@ class IRModuleNode : public Object {
   Map<GlobalVar, BaseFunc> functions;
   /*! \brief A map from global type vars to ADT type data. */
   Map<GlobalTypeVar, TypeData> type_definitions;
+  /*! \brief The external module containing the external functions used by this module. */
+  Array<runtime::Module> external_mods;

Review comment:
       As @electriclilies says we already have this, and it is already essential to making IRModule the carrier for all passes, including the existing BYOC (primitive function to runtime Module) and the TIRToRuntime extension point from @Mousius .
   
   Perhaps you are suggesting we need a way to represent 'already compiled' functions that's not bound up with the 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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] junrushao1994 commented on a change in pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

junrushao1994 commented on a change in pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#discussion_r767980273



##########
File path: include/tvm/ir/module.h
##########
@@ -57,6 +57,8 @@ class IRModuleNode : public Object {
   Map<GlobalVar, BaseFunc> functions;
   /*! \brief A map from global type vars to ADT type data. */
   Map<GlobalTypeVar, TypeData> type_definitions;
+  /*! \brief The external module containing the external functions used by this module. */
+  Array<runtime::Module> external_mods;

Review comment:
       Not sure if it's a right way to do so by fixing IR with 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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] junrushao1994 commented on pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

junrushao1994 commented on pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#issuecomment-992910847


   I'm writing some discussion on the technical side. (bear with me i'm a slow typer) BTW, It would be great if significant IR changes could go through the RFC process.


-- 
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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] junrushao1994 edited a comment on pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

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

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.

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.

## Design Discussion

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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] electriclilies commented on a change in pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

electriclilies commented on a change in pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#discussion_r768016573



##########
File path: include/tvm/ir/module.h
##########
@@ -57,6 +57,8 @@ class IRModuleNode : public Object {
   Map<GlobalVar, BaseFunc> functions;
   /*! \brief A map from global type vars to ADT type data. */
   Map<GlobalTypeVar, TypeData> type_definitions;
+  /*! \brief The external module containing the external functions used by this module. */
+  Array<runtime::Module> external_mods;

Review comment:
       @comaniac It would be great if we could have a discussion about this instead of you just saying it's not the correct approach. 
   
   Currently, the external modules are stored in the IRModule attributes, so they are already mixed, it's just not made explicit in the AST. This change just moves the external modules out of the attributes and into the IRModule as a field. 




-- 
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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mbs-octoml edited a comment on pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

mbs-octoml edited a comment on pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#issuecomment-992994783


   Thanks @junrushao1994. I'd be up for anything that replaces runtime::Module in IRModule with something more structured that nevertheless can still carry artifacts at the various stages of compilation. Any help with context much appreciated. My rough sense was we're (ab)using runtime::Module as common root for things better derived from some class that represents compiler artifacts for extern definitions without committing to any packed funcs for those artifacts.
   


-- 
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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] junrushao1994 edited a comment on pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

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 about architectural changes. These discussions makes TVM better.

## Motivation of This PR

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.

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.

## Design Discussion

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 the optional one in IRModule.attrs 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 bring broad impact that affects other ongoing parallel developments. 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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] zhiics commented on pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

zhiics commented on pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#issuecomment-993172582


   @mikepapadim Thanks for the PR. Could you elaborate a bit the motivation of moving the external runtime module into the IRModule? IMOH, the IRModule, particularly the relay IR here, doesn't really have information about the runtime stuff. We start to care about it when we are lowering the IR. I agree with @junrushao1994 that this would be worth an RFC 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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] junrushao1994 commented on pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] junrushao1994 edited a comment on pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

junrushao1994 edited a comment on pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#issuecomment-993208836

## Motivation of This PR

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.

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.

## Design Discussion

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.

--
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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [tvm] mbs-octoml commented on pull request #9729: [CORE] Make external mods first class field in the IR module

Posted by GitBox <gi...@apache.org>.

mbs-octoml commented on pull request #9729:
URL: https://github.com/apache/tvm/pull/9729#issuecomment-992994783


   Thanks @junrushao1994. I'd be up for anything that replaces runtime::Module in IRModule with something more structured that nevertheless can still carry artifacts at the various stages of compilation. Any help with context much appreciated. My rough sense was we're (ab)using runtime::Module as common root for things better derived from some class that represents compiler artifacts for extern definitions without committing to any packed funcs for those artifacts. Perhaps that's what 'metadata module' is for. 


-- 
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-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org