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/08/14 01:15:52 UTC

[GitHub] [tvm] electriclilies opened a new pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

electriclilies opened a new pull request #8750:
URL: https://github.com/apache/tvm/pull/8750


   This PR is part of some work I am doing to remove LoweredModule. We need to have attributes on the IRModules as well as on BaseFuncs to be able to remove it. Adding DictAttrs to IRModule required refactoring some of the attribute utility functions. 
   
   Previously, GetAttr and HasNonzerAttr were members of BaseFunc. I moved them to be members of DictAttr so that they can also be used for the DictAttr in the IRModule. Finally, I changed WithAttrs so that it can also work for IRModules.
   
   cc @jroesch @mbs-octoml @tkonolige @michalpiszczek 


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

Thanks Lily! I agree to have attributes on the `IRModule` level, so that we could add some metadata and guide the compiler behavior. It is consistent with the direction we are moving towards as a unified IR.

<del>However, I have some concerns on whether we should remove `BaseFunc`-level annotations. </del> Sorry I misread! I thought "BaseFuncs to be able to remove it" means removing attributes in `BaseFunc`, but it turns out to refer to other things :-)

Another thing, which I understand it is not a problem introduced by this PR, but would love to discuss a little bit if this design is the direction we want to move forward or move away, so no action needed in this PR :-) I am not so sure about is `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactored the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`.

Please let me know what you think :-)

--
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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


   LGTM.
   
   Returning to Junru's q: "would love to discuss a little bit if this design is the direction we want to move forward or move away from: I am not so sure about DictAttrs itself." I like that CallNode uses Attrs which can be subclassed to be the exact structure we care about (eg OnDeviceAttrs). Since transformations need to be loosely coupled we can't really ask IRModule to be similarly statically typed. So yeah, it sure does look like a plain old Map<...>. But I do like the WithAttrs pattern, and we can get used to that being the usual way of extending attributes.
   


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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


   Thanks Lily! I agree to have attributes on the `IRModule` level, so that we could add some metadata and guide the compiler behavior. It is consistent with the direction we are moving towards as a unified IR.
   
   <del>However, I have some concerns on whether we should remove `BaseFunc`-level annotations. </del> Sorry I misread! I thought "BaseFuncs to be able to remove it" means removing attributes in `BaseFunc`, but it turns out to refer to other things :-)
   
   One thing, which I understand it is not introduced by this PR (so no action needed in this PR :-) ), but would love to discuss a little bit if this design is the direction we want to move forward or move away from: I am not so sure about `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactored the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`. 
   
   BTW, just a readability question: shall we move `GetAttr`/`HasNonZeroAttr` back to the `BaseFunc`/`IRModule` themselves? This could sort of reduce one level of indirection when using these objects which are considered to be objects that "come with attributes" :-) We may either duplicate the code, and if we don't like duplication, it is doable via either defining a commonly used utility function, or CRTP:
   
   ```C++
   template <class TAttrObject>
   struct AttrObject {
     GetAttr(...) { 
       const DictAttrs& attrs = static_cast<TAttrObject*>(this)->get()->attrs;
       ....
     }
     HasNonZeroAttr(...) ...
   };
   
   class IRModule : public runtime::Object, public AttrObject<IRModule> {
     ...
   };
   ```
   
   
   Please let me know what you think :-)


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

However, I have some concerns on whether we should remove `BaseFunc`-level annotations. Generally speaking, the attributes we are using on `BaseFunc`-level include:
- BaseFunc: https://github.com/apache/tvm/blob/main/include/tvm/ir/function.h#L204
- TIR PrimFunc: https://github.com/apache/tvm/blob/main/include/tvm/tir/function.h#L233
- Relay Function: https://github.com/apache/tvm/blob/main/include/tvm/relay/function.h#L125

Several of the above are actually pretty to `BaseFunc`-level, not quite suitable as `IRModule`-level attributes. The particular cases I am thinking of are:
- global_symbol: used on TIR's `PrimFunc` to indicate which global symbol this function is compiled to.
- noalias: used on TIR's `PrimFunc` to indicate if the input buffers do not alias with each other

Another thing I am not so sure about is `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactor the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`. I understand it is not a problem introduced by this PR, but would love to discuss a little bit if this design is the direction we want to move forward or move away :-)

Please let me know what you think. Also @jroesch @tqchen would you guys share some thoughts on the design?

--
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] tqchen commented on a change in pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

tqchen commented on a change in pull request #8750:
URL: https://github.com/apache/tvm/pull/8750#discussion_r689492820



##########
File path: include/tvm/ir/attrs.h
##########
@@ -232,6 +233,72 @@ class DictAttrs : public Attrs {
    */
   TVM_DLL explicit DictAttrs(Map<String, ObjectRef> dict);
 
+  // Utils for accessing attributes
+  // This needs to be on DictAttrs, not DictAttrsNode because we return the default
+  // value if DictAttrsNode is not defined.
+  /*!
+   * \brief Get a function attribute.
+   *
+   * \param attr_key The attribute key.
+   * \param default_value The default value if the key does not exist, defaults to nullptr.
+   *
+   * \return The result
+   *
+   * \tparam TOBjectRef the expected object type.
+   * \throw Error if the key exists but the value does not match TObjectRef
+   *
+   * \code
+   *
+   *  void GetAttrExample(const BaseFunc& f) {
+   *    auto value = f->attrs.GetAttr<Integer>("AttrKey", 0);

Review comment:
       would be great to keep the function GetAttr and HasNonZeroAttr function in BaseFunc and IRModule, based on the considerations:
   - C0: It will remove one level of indirection(f->GetAttr vs f->attrs.GetAttr) and gives more clear documentation(since developer usually looks up doc on the Function or IRModule themselves)
   - C1: API consitency: WithAttr directly operates on the function and module, and the functions with related functionalities should ideally be made consistent with this usage.
   - C2: If there is a future refactor that changes DictAttr => Map, the API can be made consistent in a backward compaitble way
   
   We can of course keep the common impl as well and redirect in the function and module case.
   
   




-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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


   Oh I found myself misread `LoweredModule` (in TECompiler) as `LowerModule` (in driver_api.cc) 🤦‍♂️ sorry!


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

Several of the above are actually pretty specific to `BaseFunc`-level, not quite suitable as `IRModule`-level attributes. The particular cases I am thinking of are:
- global_symbol: used on TIR's `PrimFunc` to indicate which global symbol this function is compiled to.
- noalias: used on TIR's `PrimFunc` to indicate if the input buffers do not alias with each other

Another thing I am not so sure about is `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactored the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`. I understand it is not a problem introduced by this PR, but would love to discuss a little bit if this design is the direction we want to move forward or move away :-)

Please let me know what you think. Also @jroesch @tqchen would you guys share some thoughts on the design?

--
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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


   Oh I found myself misread `LoweredModule` as `LowerModule` (in driver_api.cc) 🤦‍♂️ sorry!


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

BTW, as you mentioned in the description, you might remove `LoweredModule` in the future. Perhaps I don't have full context here, but I assume it is used for lowering an IRModule, which is consistent with the direction our compiler is moving towards ("IRModule => IRModule" transformation). Would you like to elaborate (if you have time) why we are removing this method? Thanks a lot!

Please let me know what you think :-)

--
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 pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

electriclilies commented on pull request #8750:
URL: https://github.com/apache/tvm/pull/8750#issuecomment-899807585


   @tqchen does this look good to you?


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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



##########
File path: include/tvm/target/target.h
##########
@@ -54,7 +54,7 @@ class TargetNode : public Object {
   /*! \brief Keys for this target */
   Array<String> keys;
   /*! \brief Collection of attributes */
-  Map<String, ObjectRef> attrs;
+  Map<String, ObjectRef> attrs;  // TODO(@electriclilies): Unify with DictAttrs on IRModule

Review comment:
       I'm not sure if this is needed, I added it as part of my initial read through of this section of the codebase. I'll remove it and if it's needed we can discuss in a later PR




-- 
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 pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

electriclilies commented on pull request #8750:
URL: https://github.com/apache/tvm/pull/8750#issuecomment-900466741


   Thanks everyone!


-- 
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] tqchen commented on a change in pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

tqchen commented on a change in pull request #8750:
URL: https://github.com/apache/tvm/pull/8750#discussion_r689494759



##########
File path: include/tvm/target/target.h
##########
@@ -54,7 +54,7 @@ class TargetNode : public Object {
   /*! \brief Keys for this target */
   Array<String> keys;
   /*! \brief Collection of attributes */
-  Map<String, ObjectRef> attrs;
+  Map<String, ObjectRef> attrs;  // TODO(@electriclilies): Unify with DictAttrs on IRModule

Review comment:
       Target attrs is different from the IRModule attrs. They contain attributes about a target(cuda) that can be shared across IRModule or functions.
   
   Please double check if we really want to remove the target.attrs(my guess is that it is unlikely) would be great to confirm with the original authors of the target (e.g. @zxybazh )

##########
File path: include/tvm/ir/attrs.h
##########
@@ -232,6 +233,72 @@ class DictAttrs : public Attrs {
    */
   TVM_DLL explicit DictAttrs(Map<String, ObjectRef> dict);
 
+  // Utils for accessing attributes
+  // This needs to be on DictAttrs, not DictAttrsNode because we return the default
+  // value if DictAttrsNode is not defined.
+  /*!
+   * \brief Get a function attribute.
+   *
+   * \param attr_key The attribute key.
+   * \param default_value The default value if the key does not exist, defaults to nullptr.
+   *
+   * \return The result
+   *
+   * \tparam TOBjectRef the expected object type.
+   * \throw Error if the key exists but the value does not match TObjectRef
+   *
+   * \code
+   *
+   *  void GetAttrExample(const BaseFunc& f) {
+   *    auto value = f->attrs.GetAttr<Integer>("AttrKey", 0);

Review comment:
       would be great to keep the function GetAttr and HasNonZeroAttr function in BaseFunc and IRModule. For two considerations:
   - C0: It will remove one level of indirection(f->GetAttr vs f->attrs.GetAttr) and gives more clear documentation
   - C1: API consitency: WithAttr directly operates on the function and module, and the functions with related functionalities should ideally be made consistent with this usage.
   - C2: If there is a future refactor that changes DictAttr => Map, the API can be made consistent in a backward compaitble way
   
   We can of course keep the common impl as well and redirect in the function and module case.
   
   




-- 
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] tqchen commented on pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

tqchen commented on pull request #8750:
URL: https://github.com/apache/tvm/pull/8750#issuecomment-899680793


   Thanks @electriclilies "One possibility is to add GetAttrback as a method on BaseFunc and IRModule, but have that method just call DictAttr's GetAttr method." is exactly what I meant by "keep a common impl as well and redirect in the function and module case"


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

Another thing I am not so sure about is `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactored the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`. I understand it is not a problem introduced by this PR, but would love to discuss a little bit if this design is the direction we want to move forward or move away :-)

Please let me know what you think :-)

--
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

Another thing I am not so sure about is `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactored the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`. I understand it is not a problem introduced by this PR, but would love to discuss a little bit if this design is the direction we want to move forward or move away :-)

Please let me know what you think. Also @jroesch @tqchen would you guys share some thoughts on the design?

--
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 pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

electriclilies commented on pull request #8750:
URL: https://github.com/apache/tvm/pull/8750#issuecomment-899679386


   @junrushao1994 @tqchen Thanks for the feedback.
   
   To me, it makes more sense that `GetAttr` is a method on `DictAttrs`. When it's a standalone method on functions and modules, it's not clear what the attributes that GetAttr is accessing actually are. 
   
   Also, it'll still show up as a method on `DictAttrs` within the FunctionNode, so people will still be able to see it. 
   
   One possibility is to add `GetAttr`back as a method on `BaseFunc` and `IRModule`, but have that method just call `DictAttr`'s `GetAttr` method.
   
   


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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


   Thanks @electriclilies @mbs-octoml @tqchen!


-- 
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] tqchen edited a comment on pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

tqchen edited a comment on pull request #8750:
URL: https://github.com/apache/tvm/pull/8750#issuecomment-899680793


   Thanks @electriclilies "One possibility is to add GetAttrback as a method on BaseFunc and IRModule, but have that method just call DictAttr's GetAttr method." is exactly what I meant by "keep a common impl as well and redirect in the function and module case".
   
   I get your take on the need of a standalone common method, which in some sense can be seen as a tradeoff in the C0 listed above.  The addiitional considetations  "C1: API consitency: " and C2 puts a bit more weight on explicit functions on the explicit API on the function/module side.


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

Another thing I am not so sure about is `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactored the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`. I understand it is not a problem introduced by this PR, but would love to discuss a little bit if this design is the direction we want to move forward or move away :-)

BTW, as you mentioned in the description, you might remove `LoweredModule` in the future. Perhaps I don't have full context here, but I assume it is used for lowering an IRModule as our compiler is moving towards "IRModule => IRModule" transformation. Would you like to elaborate (if you have time) why we are removing this method? Thanks a lot!

Please let me know what you think. Also @jroesch @tqchen would you guys share some thoughts on the design?

--
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] tqchen commented on a change in pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

tqchen commented on a change in pull request #8750:
URL: https://github.com/apache/tvm/pull/8750#discussion_r689492820



##########
File path: include/tvm/ir/attrs.h
##########
@@ -232,6 +233,72 @@ class DictAttrs : public Attrs {
    */
   TVM_DLL explicit DictAttrs(Map<String, ObjectRef> dict);
 
+  // Utils for accessing attributes
+  // This needs to be on DictAttrs, not DictAttrsNode because we return the default
+  // value if DictAttrsNode is not defined.
+  /*!
+   * \brief Get a function attribute.
+   *
+   * \param attr_key The attribute key.
+   * \param default_value The default value if the key does not exist, defaults to nullptr.
+   *
+   * \return The result
+   *
+   * \tparam TOBjectRef the expected object type.
+   * \throw Error if the key exists but the value does not match TObjectRef
+   *
+   * \code
+   *
+   *  void GetAttrExample(const BaseFunc& f) {
+   *    auto value = f->attrs.GetAttr<Integer>("AttrKey", 0);

Review comment:
       would be great to keep the function GetAttr and HasNonZeroAttr function in BaseFunc and IRModule, based on the considerations:
   - C0: It will remove one level of indirection(f->GetAttr vs f->attrs.GetAttr) and gives more clear documentation(since developer usually looks up doc on the Function or IRModule themselves)
   - C1: API consitency: WithAttr directly operates on the function and module, and the functions with related functionalities should ideally be made consistent with this usage.
   - C2: If there is a future refactor that changes DictAttr => Map, the API can be made consistent in a backward compaitble way
   
   We can of course keep a common impl as well and redirect in the function and module case.
   
   




-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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


   @mbs-octoml Thanks for the discussion!
   
   Yes, I do like the fact that `DictAttrs` is a subclass of `Attrs`, and could be handled in a unified way as other `Attrs` on operators, etc. It provides other advantages like easier serialization.
   
   `WithAttrs` is definitely a good point, and it's not actually restricted to `DictAttrs`. If we use `Map<String, ObjectRef>` as the attributes' type, the same method is still valid with proper template programming.
   
   What about this? If an object has an `attrs` field whose type is `DictAttrs`, then we require the object to be consistent with a C++ concept that:
   - works with `WithAttr`
   - has the method `GetAttr` (with proper type signature)
   - has the method `HasNonzeroAttr` (with proper type signature)
   
   
   
   


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

One thing, which I understand it is not introduced by this PR (so no action needed in this PR :-) ), but would love to discuss a little bit if this design is the direction we want to move forward or move away from: I am not so sure about `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactored the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`.

BTW, just a readability question: shall we move `GetAttr`/`HasNonZeroAttr` back to the `BaseFunc`/`IRModule` themselves? This could sort of reduce one level of indirection when using these objects which are considered to be objects that "come with attributes" :-) To avoid code duplication, we could use CRTP to make it happen:

Please let me know what you think :-)

--
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

<del>However, I have some concerns on whether we should remove `BaseFunc`-level annotations. </del> Sorry I misread!

Another thing I am not so sure about is `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactored the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`. I understand it is not a problem introduced by this PR, but would love to discuss a little bit if this design is the direction we want to move forward or move away :-)

Please let me know what you think :-)

--
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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



##########
File path: include/tvm/target/target.h
##########
@@ -54,7 +54,7 @@ class TargetNode : public Object {
   /*! \brief Keys for this target */
   Array<String> keys;
   /*! \brief Collection of attributes */
-  Map<String, ObjectRef> attrs;
+  Map<String, ObjectRef> attrs;  // TODO(@electriclilies): Unify with DictAttrs on IRModule

Review comment:
       Target has their own attributions, for example, compute version for cuda devices. Would you like to elaborate the reason why we need to unify we need to unify this attrs into IRModule?

##########
File path: include/tvm/target/target.h
##########
@@ -54,7 +54,7 @@ class TargetNode : public Object {
   /*! \brief Keys for this target */
   Array<String> keys;
   /*! \brief Collection of attributes */
-  Map<String, ObjectRef> attrs;
+  Map<String, ObjectRef> attrs;  // TODO(@electriclilies): Unify with DictAttrs on IRModule

Review comment:
       Target has its own attributions, for example, compute version for cuda devices. Would you like to elaborate the reason why we need to unify we need to unify this attrs into IRModule?




-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

Another thing I am not so sure about is `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactored the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`. I understand it is not a problem introduced by this PR, but would love to discuss a little bit if this design is the direction we want to move forward or move away :-)

BTW, as you mentioned in the description, you might remove `LoweredModule` in the future. Perhaps I don't have full context here, but I assume it is used for lowering an IRModule, which is consistent with the direction our compiler is moving towards, the "IRModule => IRModule" transformation. Would you like to elaborate (if you have time) why we are removing this method? Thanks a lot!

Please let me know what you think. Also @jroesch @tqchen would you guys share some thoughts on the design?

--
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 edited a comment on pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

electriclilies edited a comment on pull request #8750:
URL: https://github.com/apache/tvm/pull/8750#issuecomment-899679386


   @junrushao1994 @tqchen Thanks for the feedback.
   
   To me, it makes more sense that `GetAttr` is a method on `DictAttrs`. When it's a standalone method on functions and modules, it's not clear what the attributes that GetAttr is accessing actually are. 
   
   Also, it'll still show up as a method on `DictAttrs` within the FunctionNode, so people will still be able to see it. 
   
   One possibility is to add `GetAttr`back as a method on `BaseFunc` and `IRModule`, but have that method just call `DictAttr`'s `GetAttr` method. What do you think about that?
   
   


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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


   Thanks Lily! I agree to have attributes on the `IRModule` level, so that we could add some metadata and guide the compiler behavior. It is consistent with the direction we are moving towards as a unified IR.
   
   <del>However, I have some concerns on whether we should remove `BaseFunc`-level annotations. </del> Sorry I misread! I thought "BaseFuncs to be able to remove it" means removing attributes in `BaseFunc`, but it turns out to refer to other things :-)
   
   One thing, which I understand it is not introduced by this PR (so no action needed in this PR :-) ), but would love to discuss a little bit if this design is the direction we want to move forward or move away from: I am not so sure about `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactored the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`. 
   
   BTW, just a readability question: shall we move `GetAttr`/`HasNonZeroAttr` back to the `BaseFunc`/`IRModule` themselves? This could sort of reduce one level of indirection when using these objects which are considered to be objects that "come with attributes" :-) We may either duplicate the code, and if we don't like duplication, it is doable via either defining a commonly used utility function, or CRTP:
   
   ```C++
   template <class TAttrObject>
   struct AttrObject {
     GetAttr(...) { 
       const DictAttrs& attrs = static_cast<TAttrObject*>(this)->get()->attrs;
       ....
     }
     HasNonZeroAttr(...) ...
   };
   
   class IRModule : public runtime::Object, AttrObject<IRModule> {
     ...
   };
   ```
   
   
   Please let me know what you think :-)


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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


   Thanks Lily! I agree to have attributes on the `IRModule` level, so that we could add some metadata and guide the compiler behavior. It is consistent with the direction we are moving towards as a unified IR.
   
   <del>However, I have some concerns on whether we should remove `BaseFunc`-level annotations. </del> Sorry I misread! I thought "BaseFuncs to be able to remove it" means removing attributes in `BaseFunc`, but it turns out to refer to other things :-)
   
   One thing, which I understand it is not introduced by this PR (so no action needed in this PR :-) ), but would love to discuss a little bit if this design is the direction we want to move forward or move away from: I am not so sure about `DictAttrs` itself. From my PoV it is basically equivalent to `Map<String, ObjectRef>`, but more ad-hoc in terms of unnecessary access control and accessor patterns. As an example, recently, we refactored the ForNode in TIR with the `Map<String, ObjectRef>` as loop annotation instead of `DictAttrs`. 
   
   BTW, just a readability question: shall we move `GetAttr`/`HasNonZeroAttr` back to the `BaseFunc`/`IRModule` themselves? This could sort of reduce one level of indirection when using these objects which are considered to be objects that "come with attributes" :-) We may either duplicate the code, and if we don't like duplication, it is doable via either defining a commonly used utility function, or CRTP:
   
   ```C++
   template <class TAttrObject>
   struct AttrObject {
     GetAttr(...) const { 
       const DictAttrs& attrs = static_cast<const TAttrObject*>(this)->get()->attrs;
       ....
     }
     HasNonZeroAttr(...) ...
   };
   
   class IRModule : public runtime::Object, public AttrObject<IRModule> {
     ...
   };
   ```
   
   
   Please let me know what you think :-)


-- 
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 pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

electriclilies commented on pull request #8750:
URL: https://github.com/apache/tvm/pull/8750#issuecomment-899681781


   @tqchen OK, thanks for clarifying, I thought you meant keep `GetAttrs` as a separate, standalone util and then have IRModule and BaseFunc call that 


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

Please let me know what you think. Also @jroesch @tqchen would you guys share some thoughts on the design?

--
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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

Please let me know what you think :-)

--
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 merged pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

junrushao1994 merged pull request #8750:
URL: https://github.com/apache/tvm/pull/8750


   


-- 
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 pull request #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

electriclilies commented on pull request #8750:
URL: https://github.com/apache/tvm/pull/8750#issuecomment-900466741


   Thanks everyone!


-- 
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 #8750: Add DictAttrs to IRModule and refactor DictAttrs utility functions

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

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


   Oh I found myself misread `LoweredModule` (in TECompiler) as `LowerModule` (in driver_api.cc) 🤦‍♂️ sorry!


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