-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NODEREF] Introduce named attribute system. #1618
Conversation
@jroesch @yzhliu @merrymercy please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me, but we sat together and talked design and implementation, maybe good to get review from others?
include/tvm/attrs.h
Outdated
* \file tvm/attrs.h | ||
* \brief TVM attribute module | ||
* | ||
* This module enables declaration named attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module enables declaration named attributes
should be:
This module enables declaration of named attributes
include/tvm/attrs.h
Outdated
* \brief TVM attribute module | ||
* | ||
* This module enables declaration named attributes | ||
* and support default value setup and bound checking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and support default value setup and bound checking.
should be:
which support default value setup and bound checking.
include/tvm/attrs.h
Outdated
/*! | ||
* \brief Declare an attribute function. | ||
* \param ClassName The name of the class. | ||
* \param TypeKey The type key to be used in the tvm node system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type key to be used by the TVM node system.
include/tvm/attrs.h
Outdated
__fvisit__(#FieldName, &FieldName) | ||
|
||
|
||
/*! \brief Error throwed during attribute checking */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error thrown during attribute checking.
include/tvm/attrs.h
Outdated
/*! \brief name of the field */ | ||
std::string name; | ||
/*! \brief type of the field in string format */ | ||
std::string type_info; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we store the type in string format? is this the string key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used in docstring, clarified in the comment
include/tvm/attrs.h
Outdated
TVM_DECLARE_NODE_TYPE_INFO(DictAttrsNode, BaseAttrsNode); | ||
}; | ||
|
||
// Namespace fof detail implementationsx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The english here is unclear.
Namespace containing detail implementations?
include/tvm/attrs.h
Outdated
} | ||
return *this; | ||
} | ||
// set upper bound checks the upper bound |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Could we make these full sentences ?
include/tvm/attrs.h
Outdated
template<typename FFind> | ||
class AttrInitVisitor { | ||
public: | ||
// hit count on visiting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain this more in depth?
python/tvm/make.py
Outdated
Returns | ||
------- | ||
node : Node | ||
The corresponding created DSL Node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is grammatically strange, maybe The corresponding DLS node.
?
include/tvm/runtime/packed_func.h
Outdated
@@ -223,6 +223,10 @@ class ExtTypeVTable { | |||
class TVMPODValue_ { | |||
public: | |||
operator double() const { | |||
// Allow automatic conversion from int to float | |||
if (type_code_ == kDLInt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this to make the attributes more flexible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for example, user can do add_one(x, 1), and in the backend we can directly fetch out a float, for functions that want to do so
@jroesch review comments address, please take another look |
👍 |
Thanks @jroesch this is now merged |
So far we introduced Map<string, NodeRef> for general attribute storage. As we plan to upgrade the high-level IR, we would like to introduce more structure into it, specifically
dmlc::Parameter
that declares the default value and attribute value propertyThis PR introduces attrs.h to solve the problem. The declaration API is relatively similar to dmlc::Parameter, and the Attrs is backed by the Node system. The general idea is that we declare the attribute in the C++, and it can be made accessible from the python side. We also introduce a DictAttrsNode that is backed by a dictionary, to mimic the python dict based object
Features
C++ Declaration
Python Side Access