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 2022/04/04 19:14:31 UTC
[GitHub] [tvm] shingjan opened a new pull request, #10892: [TVMScript] Fixing T.buffer with typed positional arguments other than int32
shingjan opened a new pull request, #10892:
URL: https://github.com/apache/tvm/pull/10892
This is a temporary fix for python not handling this case `T.Buffer[(T.int64(xx), T.int64(xx)), "float32"]` well. For now, `TVMScriptPrinter` will not print `match_buffer` with syntax sugar if the `shape` argument is typed other than `int32`.
Root cause of this behavior is that we defined `T.Buffer` as `__getitem__(self, args)` in which `args` is a list of all arguments. But adding `T.int64` to the list will make Python think there is more than one positional arguments and raise a TypeError. TVMScript with such semantics could actually pass our parser check but not the type checker of Python, meaning only handwritten script with typed args in `T.buffer` will raise this error.
@MasterJH5574 @vinx13 @junrushao1994
--
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] shingjan commented on a diff in pull request #10892: [TVMScript] Fixing T.buffer with typed positional arguments other than int32
Posted by GitBox <gi...@apache.org>.
shingjan commented on code in PR #10892:
URL: https://github.com/apache/tvm/pull/10892#discussion_r845474408
##########
src/printer/tvmscript_printer.cc:
##########
@@ -577,6 +577,9 @@ bool TVMScriptPrinter::IsSimpleBuffer(const Buffer& buf) {
return false;
}
for (const PrimExpr& shp_i : buf->shape) {
+ if (shp_i.dtype() != DataType::Int(32)) {
+ return false;
+ }
Review Comment:
The current code change is to change the definition of `ConcreteType` so it could take more than one positional arguments.
--
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] vinx13 commented on a diff in pull request #10892: [TVMScript] Fixing T.buffer with typed positional arguments other than int32
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #10892:
URL: https://github.com/apache/tvm/pull/10892#discussion_r842244293
##########
tests/python/unittest/test_tvmscript_syntax_sugar.py:
##########
@@ -181,5 +181,27 @@ def test_dynamic_shape_gemm():
assert_structural_equal(gemm_dyn_shape, gemm_dyn_shape_roundtrip)
+@T.prim_func
+def match_buffer_int64(a: T.handle, c: T.handle) -> None:
+ A = T.match_buffer(a, (T.int64(128), T.int64(128)), dtype="float32")
+ B = T.alloc_buffer((T.int64(128), T.int64(128)), dtype="float32")
+ C = T.match_buffer(c, (T.int64(128), T.int64(128)), dtype="float32")
+ for i, j in T.grid(128, 128):
+ with T.block("B"):
+ vi, vj = T.axis.remap("SS", [i, j])
+ B[vi, vj] = A[vi, vj] * 2.0
+ for i, j in T.grid(T.int64(128), T.int64(128)):
+ with T.block("C"):
+ vi, vj = T.axis.remap("SS", [i, j])
+ C[vi, vj] = B[vi, vj] + 1.0
+
+
+def test_match_buffer_int64():
+ original = match_buffer_int64
+ after_roundtrip = from_source(original.script(show_meta=True))
+ after_roundtrip_back = from_source(after_roundtrip.script(show_meta=True))
+ assert_structural_equal(after_roundtrip, after_roundtrip_back, True)
Review Comment:
this is actually more than one roundtrip, is it necessary?
--
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] vinx13 merged pull request #10892: [TVMScript] Fixing T.buffer with typed positional arguments other than int32
Posted by GitBox <gi...@apache.org>.
vinx13 merged PR #10892:
URL: https://github.com/apache/tvm/pull/10892
--
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] leandron commented on pull request #10892: [TVMScript] Fixing T.buffer with typed positional arguments other than int32
Posted by GitBox <gi...@apache.org>.
leandron commented on PR #10892:
URL: https://github.com/apache/tvm/pull/10892#issuecomment-1087954826
Hi, please rebase to get the hotfix https://github.com/apache/tvm/pull/10893 and please re-trigger CI. Thanks.
--
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] MasterJH5574 commented on a diff in pull request #10892: [TVMScript] Fixing T.buffer with typed positional arguments other than int32
Posted by GitBox <gi...@apache.org>.
MasterJH5574 commented on code in PR #10892:
URL: https://github.com/apache/tvm/pull/10892#discussion_r843365598
##########
src/printer/tvmscript_printer.cc:
##########
@@ -577,6 +577,9 @@ bool TVMScriptPrinter::IsSimpleBuffer(const Buffer& buf) {
return false;
}
for (const PrimExpr& shp_i : buf->shape) {
+ if (shp_i.dtype() != DataType::Int(32)) {
+ return false;
+ }
Review Comment:
Hi. It’s true that we can choose not to print in the sugar type annotation style when the shape contains `int64`, but what I’m curious is, can’t we enhance the type annotation to support `int64`? That way makes more sense, since we always want to make our stuff better and work in different cases.
--
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] shingjan commented on a diff in pull request #10892: [TVMScript] Fixing T.buffer with typed positional arguments other than int32
Posted by GitBox <gi...@apache.org>.
shingjan commented on code in PR #10892:
URL: https://github.com/apache/tvm/pull/10892#discussion_r845473398
##########
src/printer/tvmscript_printer.cc:
##########
@@ -577,6 +577,9 @@ bool TVMScriptPrinter::IsSimpleBuffer(const Buffer& buf) {
return false;
}
for (const PrimExpr& shp_i : buf->shape) {
+ if (shp_i.dtype() != DataType::Int(32)) {
+ return false;
+ }
Review Comment:
@MasterJH5574 The initial code change of this PR is to act as a temporary fix to address your [comment](https://github.com/apache/tvm/pull/10789#issuecomment-1079625981) 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org