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/11/18 12:43:13 UTC
[GitHub] [tvm] abhikran-quic opened a new pull request, #13429: [TIR][Schedule] Add condition to check buffer type
abhikran-quic opened a new pull request, #13429:
URL: https://github.com/apache/tvm/pull/13429
Add condition to check buffer type while creating array for padding.
This check is helpful to ensure that `buffer_obj` data type should be similar to `pad_value` type.
Co-authored by: Anirudh Sundar Subramaniam <qu...@quicinc.com>
cc: @Lunderberg
--
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] Lunderberg commented on a diff in pull request #13429: [TIR][Schedule] Add condition to check buffer type
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13429:
URL: https://github.com/apache/tvm/pull/13429#discussion_r1026554437
##########
tests/python/unittest/test_tir_schedule_transform_layout.py:
##########
@@ -533,6 +533,21 @@ def before():
expected = tvm.tir.schedule.schedule.ScheduleError
+class TestErrorOnNonMatchingTypes(BasePaddingCompare):
+ """The padding must have the same dtype as the buffer"""
+
+ pad_value = tvm.testing.parameter(0)
+
+ def before():
+ A = T.alloc_buffer(14, "float32")
+ for i in T.serial(14):
+ with T.block("block"):
+ vi = T.axis.remap("S", [i])
+ A[vi] = 0
+
+ expected = TypeError
Review Comment:
Does the behavior here differ from [TestErrorOnWrongPaddingType](https://github.com/apache/tvm/blob/main/tests/python/unittest/test_tir_schedule_transform_layout.py#L521)? Other than the type of error thrown, it looks like they are testing the same behavior.
--
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] Lunderberg commented on a diff in pull request #13429: [TIR][Schedule] Add condition to check buffer type
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13429:
URL: https://github.com/apache/tvm/pull/13429#discussion_r1026552127
##########
python/tvm/tir/schedule/schedule.py:
##########
@@ -2751,10 +2751,14 @@ def two_elementwise_transformed_intermediate_buffer(a: T.handle, c: T.handle) ->
# buffer's type. If the default `tvm.runtime.convert`
# behavior is applied, these would be converted to
# int32/float32, which may not match the buffer's type.
- if isinstance(pad_value, int):
+ if "int" in buffer_obj.dtype and isinstance(pad_value, int):
pad_value = IntImm(buffer_obj.dtype, pad_value)
- elif isinstance(pad_value, float):
+ elif "float" in buffer_obj.dtype and isinstance(pad_value, float):
pad_value = FloatImm(buffer_obj.dtype, pad_value)
+ else:
Review Comment:
I like the change overall, to avoid constructing an `IntImm` for type `float64`, but this `else` case should be removed. There's an existing check on the C++ side [here](https://github.com/apache/tvm/blob/main/src/tir/schedule/primitive/layout_transformation.cc#L1073), which validates the resulting type. The python-side type coercion is only necessary to avoid ambiguous conversions.
--
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] tvm-bot commented on pull request #13429: [TIR][Schedule] Add condition to check buffer type
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13429:
URL: https://github.com/apache/tvm/pull/13429#issuecomment-1319947153
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* cc @Hzfengsy, @junrushao <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
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] Lunderberg merged pull request #13429: [TIR][Schedule] Add condition to check buffer type
Posted by GitBox <gi...@apache.org>.
Lunderberg merged PR #13429:
URL: https://github.com/apache/tvm/pull/13429
--
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] abhikran-quic commented on a diff in pull request #13429: [TIR][Schedule] Add condition to check buffer type
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on code in PR #13429:
URL: https://github.com/apache/tvm/pull/13429#discussion_r1026749477
##########
tests/python/unittest/test_tir_schedule_transform_layout.py:
##########
@@ -533,6 +533,21 @@ def before():
expected = tvm.tir.schedule.schedule.ScheduleError
+class TestErrorOnNonMatchingTypes(BasePaddingCompare):
+ """The padding must have the same dtype as the buffer"""
+
+ pad_value = tvm.testing.parameter(0)
+
+ def before():
+ A = T.alloc_buffer(14, "float32")
+ for i in T.serial(14):
+ with T.block("block"):
+ vi = T.axis.remap("S", [i])
+ A[vi] = 0
+
+ expected = TypeError
Review Comment:
Yes. `TestErrorOnWrongPaddingType` has the buffer dtype and `pad_value` type as a variant of `int`. However, for a version of `resnet`, I encountered a corner case where buffer was of `float32` type whereas `pad_value` was being passed as `int` to `padded layout_transform` which led to failure while creating `pad_value` array using `IntImm` in the original code in `schedule.py`.
Here, I'm trying to add a test case to catch similar behavior. Instead of catching `TypeError`, I've update the code to catch `ScheduleError`
##########
python/tvm/tir/schedule/schedule.py:
##########
@@ -2751,10 +2751,14 @@ def two_elementwise_transformed_intermediate_buffer(a: T.handle, c: T.handle) ->
# buffer's type. If the default `tvm.runtime.convert`
# behavior is applied, these would be converted to
# int32/float32, which may not match the buffer's type.
- if isinstance(pad_value, int):
+ if "int" in buffer_obj.dtype and isinstance(pad_value, int):
pad_value = IntImm(buffer_obj.dtype, pad_value)
- elif isinstance(pad_value, float):
+ elif "float" in buffer_obj.dtype and isinstance(pad_value, float):
pad_value = FloatImm(buffer_obj.dtype, pad_value)
+ else:
Review Comment:
Sure. Agree with you as I was not aware of the C++ check.
--
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] abhikran-quic commented on pull request #13429: [TIR][Schedule] Add condition to check buffer type
Posted by GitBox <gi...@apache.org>.
abhikran-quic commented on PR #13429:
URL: https://github.com/apache/tvm/pull/13429#issuecomment-1328789528
@tvm-bot rerun
--
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] Lunderberg commented on a diff in pull request #13429: [TIR][Schedule] Add condition to check buffer type
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13429:
URL: https://github.com/apache/tvm/pull/13429#discussion_r1033604282
##########
tests/python/unittest/test_tir_schedule_transform_layout.py:
##########
@@ -533,6 +533,21 @@ def before():
expected = tvm.tir.schedule.schedule.ScheduleError
+class TestErrorOnNonMatchingTypes(BasePaddingCompare):
+ """The padding must have the same dtype as the buffer"""
+
+ pad_value = tvm.testing.parameter(0)
+
+ def before():
+ A = T.alloc_buffer(14, "float32")
+ for i in T.serial(14):
+ with T.block("block"):
+ vi = T.axis.remap("S", [i])
+ A[vi] = 0
+
+ expected = TypeError
Review Comment:
Ah, got it. A test of usage error where an entirely incompatible type is being passed, rather than just a test that requires a different Python to TIR conversion. Thank you!
--
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