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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
