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/01/18 18:20:17 UTC
[GitHub] [tvm] tqchen opened a new pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
tqchen opened a new pull request #7306:
URL: https://github.com/apache/tvm/pull/7306
ForNode update
- Remove deprecated device_api.
- Add ThreadBinding for_type.
- Add additional annotations.
More style consistency refactor to make the ForNode
to be consistent with rest of the codebase.
- ForType => ForKind
- Add constant prefix k to enum consts per Google C style
- Introduce ForKind to the python 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Hzfengsy commented on a change in pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
Posted by GitBox <gi...@apache.org>.
Hzfengsy commented on a change in pull request #7306:
URL: https://github.com/apache/tvm/pull/7306#discussion_r559847680
##########
File path: python/tvm/tir/ir_builder.py
##########
@@ -249,17 +249,17 @@ def for_range(self, begin, end, name="i", dtype="int32", for_type="serial"):
extent = end if begin == 0 else (end - begin)
def _exit_cb():
- if for_type == "serial":
- for_type_id = 0
- elif for_type == "parallel":
- for_type_id = 1
- elif for_type == "vectorize":
- for_type_id = 2
- elif for_type == "unroll":
- for_type_id = 3
+ if kind == "serial":
+ kind_id = 0
+ elif kind == "parallel":
+ kind_id = 1
+ elif kind == "vectorize":
+ kind_id = 2
+ elif kind == "unroll":
+ kind_id = 3
Review comment:
Shall we add thread_binding here?
----------------------------------------------------------------
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
[GitHub] [tvm] tqchen commented on a change in pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7306:
URL: https://github.com/apache/tvm/pull/7306#discussion_r559854513
##########
File path: python/tvm/tir/ir_builder.py
##########
@@ -249,17 +249,17 @@ def for_range(self, begin, end, name="i", dtype="int32", for_type="serial"):
extent = end if begin == 0 else (end - begin)
def _exit_cb():
- if for_type == "serial":
- for_type_id = 0
- elif for_type == "parallel":
- for_type_id = 1
- elif for_type == "vectorize":
- for_type_id = 2
- elif for_type == "unroll":
- for_type_id = 3
+ if kind == "serial":
+ kind_id = 0
+ elif kind == "parallel":
+ kind_id = 1
+ elif kind == "vectorize":
+ kind_id = 2
+ elif kind == "unroll":
+ kind_id = 3
Review comment:
We are trying to be consistent with the old API for now. So let us keep it as it is.
----------------------------------------------------------------
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
[GitHub] [tvm] junrushao1994 merged pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #7306:
URL: https://github.com/apache/tvm/pull/7306
----------------------------------------------------------------
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
[GitHub] [tvm] tqchen commented on a change in pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7306:
URL: https://github.com/apache/tvm/pull/7306#discussion_r559850340
##########
File path: python/tvm/tir/ir_builder.py
##########
@@ -249,17 +249,17 @@ def for_range(self, begin, end, name="i", dtype="int32", for_type="serial"):
extent = end if begin == 0 else (end - begin)
def _exit_cb():
- if for_type == "serial":
- for_type_id = 0
- elif for_type == "parallel":
- for_type_id = 1
- elif for_type == "vectorize":
- for_type_id = 2
- elif for_type == "unroll":
- for_type_id = 3
+ if kind == "serial":
+ kind_id = 0
+ elif kind == "parallel":
+ kind_id = 1
+ elif kind == "vectorize":
+ kind_id = 2
+ elif kind == "unroll":
+ kind_id = 3
Review comment:
thread binding is not yet supported formally as an API, so we should add it later once we formally land the language support
----------------------------------------------------------------
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
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7306:
URL: https://github.com/apache/tvm/pull/7306#discussion_r559874698
##########
File path: src/printer/tir_text_printer.cc
##########
@@ -465,27 +465,30 @@ Doc TIRTextPrinter::VisitStmt_(const EvaluateNode* op) {
return doc;
}
-inline const char* ForType2String(ForType t) {
+inline const char* ForKind2String(ForKind t) {
Review comment:
Consider move this to tir/stmt.h?
##########
File path: src/printer/tir_text_printer.cc
##########
@@ -465,27 +465,30 @@ Doc TIRTextPrinter::VisitStmt_(const EvaluateNode* op) {
return doc;
}
-inline const char* ForType2String(ForType t) {
+inline const char* ForKind2String(ForKind t) {
Review comment:
Consider moving this to tir/stmt.h?
----------------------------------------------------------------
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
[GitHub] [tvm] tqchen commented on a change in pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7306:
URL: https://github.com/apache/tvm/pull/7306#discussion_r559880981
##########
File path: src/printer/tir_text_printer.cc
##########
@@ -465,27 +465,30 @@ Doc TIRTextPrinter::VisitStmt_(const EvaluateNode* op) {
return doc;
}
-inline const char* ForType2String(ForType t) {
+inline const char* ForKind2String(ForKind t) {
Review comment:
This case this is specifically used in text printer, so we plan to keep it here 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
[GitHub] [tvm] jcf94 commented on a change in pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
Posted by GitBox <gi...@apache.org>.
jcf94 commented on a change in pull request #7306:
URL: https://github.com/apache/tvm/pull/7306#discussion_r559852134
##########
File path: python/tvm/tir/ir_builder.py
##########
@@ -249,17 +249,17 @@ def for_range(self, begin, end, name="i", dtype="int32", for_type="serial"):
extent = end if begin == 0 else (end - begin)
def _exit_cb():
- if for_type == "serial":
- for_type_id = 0
- elif for_type == "parallel":
- for_type_id = 1
- elif for_type == "vectorize":
- for_type_id = 2
- elif for_type == "unroll":
- for_type_id = 3
+ if kind == "serial":
+ kind_id = 0
+ elif kind == "parallel":
+ kind_id = 1
+ elif kind == "vectorize":
+ kind_id = 2
+ elif kind == "unroll":
+ kind_id = 3
Review comment:
```suggestion
if kind == "serial":
kind_id = ForKind.SERIAL
elif kind == "parallel":
kind_id = ForKind.PARALLEL
elif kind == "vectorize":
kind_id = ForKind.VECTORIZED
elif kind == "unroll":
kind_id = ForKind.UNROLLED
```
Will it be better to also use ForKind as the input of this function?
----------------------------------------------------------------
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
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7306:
URL: https://github.com/apache/tvm/pull/7306#discussion_r559875789
##########
File path: tests/python/unittest/test_tir_transform_loop_partition.py
##########
@@ -462,6 +462,7 @@ def test_multilevel_splitting_with_indivisble_factors():
def visit_stmt(op):
return isinstance(op, tvm.tir.Max)
+ print(lowered_body)
Review comment:
Remove this line?
----------------------------------------------------------------
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
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7306:
URL: https://github.com/apache/tvm/pull/7306#discussion_r559919460
##########
File path: src/printer/tir_text_printer.cc
##########
@@ -465,27 +465,30 @@ Doc TIRTextPrinter::VisitStmt_(const EvaluateNode* op) {
return doc;
}
-inline const char* ForType2String(ForType t) {
+inline const char* ForKind2String(ForKind t) {
Review comment:
I thought our convention is do define a stringify method immediately after defining an enum class...
----------------------------------------------------------------
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
[GitHub] [tvm] junrushao1994 commented on pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7306:
URL: https://github.com/apache/tvm/pull/7306#issuecomment-762644052
Thanks @tqchen @jcf94 @spectrometerHBH! It is merged :-)
----------------------------------------------------------------
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
[GitHub] [tvm] tqchen commented on a change in pull request #7306: [TIR][REFACTOR] ForNode introduce thread binding and remove legacy field
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7306:
URL: https://github.com/apache/tvm/pull/7306#discussion_r559881021
##########
File path: tests/python/unittest/test_tir_transform_loop_partition.py
##########
@@ -462,6 +462,7 @@ def test_multilevel_splitting_with_indivisble_factors():
def visit_stmt(op):
return isinstance(op, tvm.tir.Max)
+ print(lowered_body)
Review comment:
Good catch
----------------------------------------------------------------
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