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 2020/01/22 01:55:41 UTC
[GitHub] [incubator-tvm] yzhliu opened a new pull request #4765: [tir] make
IterVar extends Var
yzhliu opened a new pull request #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765
@tqchen @ZihengJiang @Hzfengsy Please review
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] yzhliu closed pull request #4765: [tir] make
IterVar extends Var
Posted by GitBox <gi...@apache.org>.
yzhliu closed pull request #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] yzhliu commented on issue #4765: [tir] make IterVar
extends Var
Posted by GitBox <gi...@apache.org>.
yzhliu commented on issue #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765#issuecomment-578281104
@tqchen another problem is currently we rely on `IterVar->var` to propagate variables from the very beginning, e.g.,
```
rv = tvm.reduce_axis(...)
Y = sum(X[rv], axis=rv)
```
However, `rv` could be replaced during the pass, it cause problem when we use the original `rv` to access array
one example of replacing: https://github.com/apache/incubator-tvm/blob/master/src/tir/ir/expr_functor.cc#L206-L208
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] yzhliu edited a comment on issue #4765: [tir] make
IterVar extends Var
Posted by GitBox <gi...@apache.org>.
yzhliu edited a comment on issue #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765#issuecomment-578281104
@tqchen another problem is currently we rely on `IterVar->var` to propagate variables from the very beginning, e.g.,
```
rv = tvm.reduce_axis(...)
Y = sum(X[rv], axis=rv)
```
However, `rv` could be replaced during the pass, it cause problem when we use the original `rv` to access array
one example of replacing: https://github.com/apache/incubator-tvm/blob/master/src/tir/ir/expr_functor.cc#L206-L208
rethink about the scenario, I feel it is actually not a bad idea to have `Var` as a member of `IterVar`, it simplifies the problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4765:
[tir] make IterVar extends Var
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765#discussion_r370429075
##########
File path: include/tvm/tir/expr.h
##########
@@ -236,21 +228,33 @@ enum IterVarType : int {
* \brief Iteration Variable,
* represents an iteration over an integer interval.
*/
-class IterVar : public ObjectRef {
+class IterVar : public Var {
public:
// construct a new iter var without a domain
IterVar() {}
// construct from shared ptr.
- explicit IterVar(ObjectPtr<Object> n) : ObjectRef(n) {}
+ explicit IterVar(ObjectPtr<Object> n) : Var(n) {}
+ /*! \brief constructor.
+ * \param dom interval of the variable.
+ * \param iter_type indicate the iteration type of the variable.
+ * \param name_hint variable name
+ * \param t data type
+ * \param thread_tag additional tag on the iteration variable.
+ */
+ TVM_DLL IterVar(Range dom, IterVarType iter_type,
Review comment:
I feel in this case perhaps it is also worth to consider consistency withVar, which means we should put name first. But provide auxilary factory functions(e.g. thread_axis) to quickly construct some of the data
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4765:
[tir] make IterVar extends Var
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765#discussion_r369356112
##########
File path: include/tvm/tir/expr.h
##########
@@ -236,21 +228,33 @@ enum IterVarType : int {
* \brief Iteration Variable,
* represents an iteration over an integer interval.
*/
-class IterVar : public ObjectRef {
+class IterVar : public Var {
public:
// construct a new iter var without a domain
IterVar() {}
// construct from shared ptr.
- explicit IterVar(ObjectPtr<Object> n) : ObjectRef(n) {}
+ explicit IterVar(ObjectPtr<Object> n) : Var(n) {}
+ /*! \brief constructor.
+ * \param dom interval of the variable.
+ * \param iter_type indicate the iteration type of the variable.
+ * \param name_hint variable name
+ * \param t data type
+ * \param thread_tag additional tag on the iteration variable.
+ */
+ TVM_DLL IterVar(Range dom, IterVarType iter_type,
Review comment:
let us cross reference python API to check if it is the right order
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #4765:
[tir] make IterVar extends Var
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765#discussion_r369356042
##########
File path: include/tvm/tir/expr.h
##########
@@ -236,21 +228,33 @@ enum IterVarType : int {
* \brief Iteration Variable,
* represents an iteration over an integer interval.
*/
-class IterVar : public ObjectRef {
+class IterVar : public Var {
public:
// construct a new iter var without a domain
IterVar() {}
// construct from shared ptr.
- explicit IterVar(ObjectPtr<Object> n) : ObjectRef(n) {}
+ explicit IterVar(ObjectPtr<Object> n) : Var(n) {}
+ /*! \brief constructor.
+ * \param dom interval of the variable.
+ * \param iter_type indicate the iteration type of the variable.
+ * \param name_hint variable name
+ * \param t data type
+ * \param thread_tag additional tag on the iteration variable.
+ */
+ TVM_DLL IterVar(Range dom, IterVarType iter_type,
+ std::string name_hint = "v",
+ DataType t = DataType::Int(32),
+ std::string thread_tag = "");
/*!
* \brief access the internal node container
* \return the pointer to the internal node container
*/
inline const IterVarNode* operator->() const;
Review comment:
We can likely use TVM_DEFINE_OBJECT_REF_METHODS to define these methods
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] yzhliu commented on a change in pull request #4765:
[tir] make IterVar extends Var
Posted by GitBox <gi...@apache.org>.
yzhliu commented on a change in pull request #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765#discussion_r369754812
##########
File path: src/tir/ir/expr.cc
##########
@@ -45,24 +45,28 @@ SizeVar::SizeVar(std::string name_hint, DataType t)
SizeVarNode::SizeVarNode(DataType t, std::string name_hint)
: VarNode(t, std::move(name_hint)) {}
-IterVar IterVarNode::make(Range dom,
- Var var,
- IterVarType t,
- std::string thread_tag) {
- ObjectPtr<IterVarNode> n = make_object<IterVarNode>();
- n->dom = dom;
- n->var = var;
- n->iter_type = t;
- n->thread_tag = thread_tag;
- return IterVar(n);
+IterVarNode::IterVarNode(DataType dtype, std::string name_hint,
Review comment:
it's used in https://github.com/apache/incubator-tvm/pull/4765/files#diff-b0d470d0765d89448d8d18506ba0c54eR61
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] yzhliu commented on a change in pull request #4765:
[tir] make IterVar extends Var
Posted by GitBox <gi...@apache.org>.
yzhliu commented on a change in pull request #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765#discussion_r369754311
##########
File path: include/tvm/tir/expr.h
##########
@@ -236,21 +228,33 @@ enum IterVarType : int {
* \brief Iteration Variable,
* represents an iteration over an integer interval.
*/
-class IterVar : public ObjectRef {
+class IterVar : public Var {
public:
// construct a new iter var without a domain
IterVar() {}
// construct from shared ptr.
- explicit IterVar(ObjectPtr<Object> n) : ObjectRef(n) {}
+ explicit IterVar(ObjectPtr<Object> n) : Var(n) {}
+ /*! \brief constructor.
+ * \param dom interval of the variable.
+ * \param iter_type indicate the iteration type of the variable.
+ * \param name_hint variable name
+ * \param t data type
+ * \param thread_tag additional tag on the iteration variable.
+ */
+ TVM_DLL IterVar(Range dom, IterVarType iter_type,
Review comment:
in python it's `_IterVar(dom, name, iter_type, thread_tag='')`, while if we need to make `name` optional, we have to make `name` after `iter_type`, right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #4765: [tir] make IterVar
extends Var
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765#issuecomment-578369268
It is indeed tradeoff in here, it is similar to early binding of information vs late binding. @yzhliu If you think it is a good idea to keep IterVar as it is(to simplify the mutation of vars), we can close this PR for now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] yzhliu commented on issue #4765: [tir] make IterVar
extends Var
Posted by GitBox <gi...@apache.org>.
yzhliu commented on issue #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765#issuecomment-578369831
Thanks. I'll close it for now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] Hzfengsy commented on a change in pull request
#4765: [tir] make IterVar extends Var
Posted by GitBox <gi...@apache.org>.
Hzfengsy commented on a change in pull request #4765: [tir] make IterVar extends Var
URL: https://github.com/apache/incubator-tvm/pull/4765#discussion_r369340510
##########
File path: src/tir/ir/expr.cc
##########
@@ -45,24 +45,28 @@ SizeVar::SizeVar(std::string name_hint, DataType t)
SizeVarNode::SizeVarNode(DataType t, std::string name_hint)
: VarNode(t, std::move(name_hint)) {}
-IterVar IterVarNode::make(Range dom,
- Var var,
- IterVarType t,
- std::string thread_tag) {
- ObjectPtr<IterVarNode> n = make_object<IterVarNode>();
- n->dom = dom;
- n->var = var;
- n->iter_type = t;
- n->thread_tag = thread_tag;
- return IterVar(n);
+IterVarNode::IterVarNode(DataType dtype, std::string name_hint,
Review comment:
I'm not sure whether we need a constructor for Node
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services