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/07/07 21:42:59 UTC
[GitHub] [tvm] zxybazh opened a new pull request, #12035: [MetaSchedule][TIR] Patch for Data Type Inconsistency After Trace Reloading
zxybazh opened a new pull request, #12035:
URL: https://github.com/apache/tvm/pull/12035
This PR introduces a type check to cast loop split decisions (sometimes given as `int64`) back to a smaller datatype when the loop variable's data type is smaller. This issue usually happens during reloading a trace from disk using JSON database and causes the failure of `CompactBufferAllocation` pass.
Unit test is also added.
--
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] zxybazh commented on a diff in pull request #12035: [MetaSchedule][TIR] Patch for Data Type Inconsistency After Trace Reloading
Posted by GitBox <gi...@apache.org>.
zxybazh commented on code in PR #12035:
URL: https://github.com/apache/tvm/pull/12035#discussion_r916435643
##########
src/tir/schedule/concrete_schedule.cc:
##########
@@ -452,6 +452,9 @@ Array<LoopRV> ConcreteScheduleNode::Split(const LoopRV& loop_rv,
if (is_const_int(factor) && !is_positive_const(factor)) {
throw NonPositiveFactorError(state_->mod, factor.as<IntImmNode>()->value, i);
}
+ if (factor.dtype().bits() > loop->extent.dtype().bits()) {
+ factor = tir::Cast(loop->extent.dtype(), factor);
Review Comment:
Do you mean `tvm::cast` not `tir::cast` right? There seems to be no lowercased cast in tir namespace.
--
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] zxybazh commented on pull request #12035: [MetaSchedule][TIR] Patch for Data Type Inconsistency After Trace Reloading
Posted by GitBox <gi...@apache.org>.
zxybazh commented on PR #12035:
URL: https://github.com/apache/tvm/pull/12035#issuecomment-1178372789
Double checked and confirmed the only source of data type inconsistency comes from directly calling `Split` with given decisions. Therefore, I introduced a type check in `Split`'s factor parsing part. Ready for review. CC: @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] junrushao1994 commented on a diff in pull request #12035: [MetaSchedule][TIR] Patch for Data Type Inconsistency After Trace Reloading
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12035:
URL: https://github.com/apache/tvm/pull/12035#discussion_r916433151
##########
src/tir/schedule/concrete_schedule.cc:
##########
@@ -452,6 +452,9 @@ Array<LoopRV> ConcreteScheduleNode::Split(const LoopRV& loop_rv,
if (is_const_int(factor) && !is_positive_const(factor)) {
throw NonPositiveFactorError(state_->mod, factor.as<IntImmNode>()->value, i);
}
+ if (factor.dtype().bits() > loop->extent.dtype().bits()) {
+ factor = tir::Cast(loop->extent.dtype(), factor);
Review Comment:
use lower cased cast
--
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] junrushao1994 commented on a diff in pull request #12035: [TIR] Avoid unnecessary dtype escalation in loop splitting
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12035:
URL: https://github.com/apache/tvm/pull/12035#discussion_r916440640
##########
src/tir/schedule/concrete_schedule.cc:
##########
@@ -452,6 +452,9 @@ Array<LoopRV> ConcreteScheduleNode::Split(const LoopRV& loop_rv,
if (is_const_int(factor) && !is_positive_const(factor)) {
throw NonPositiveFactorError(state_->mod, factor.as<IntImmNode>()->value, i);
}
+ if (factor.dtype().bits() > loop->extent.dtype().bits()) {
+ factor = tir::Cast(loop->extent.dtype(), factor);
Review Comment:
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a diff in pull request #12035: [MetaSchedule][TIR] Patch for Data Type Inconsistency After Trace Reloading
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12035:
URL: https://github.com/apache/tvm/pull/12035#discussion_r916363627
##########
tests/python/unittest/test_meta_schedule_database.py:
##########
@@ -68,9 +76,78 @@ def main(a: T.handle, b: T.handle, d: T.handle) -> None: # pylint: disable=no-s
D[vi, vj] = T.max(C[vi, vj], 0.0)
-# fmt: on
-# pylint: enable=invalid-name,no-member,line-too-long,too-many-nested-blocks,no-self-argument
-
+@tvm.script.ir_module
+class MobileNetConv2d:
Review Comment:
first of all, this PR fixes a TIR issue, so it has nothing to do with MetaSchedule fundamentally; second, we should focus on "minimal" reproducible example while it's indeed not minimal enough.
alternatively, I would suggest
1) creating a unittest in `test_tir_schedule_split_fuse.py`
2) the IRModule contains a single loop
3) the schedule contains a single `split`, whose factor is `factors=[None, IntImm("int64", some-number)]`
4) check if the loop extent remains int32
--
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] junrushao1994 commented on a diff in pull request #12035: [MetaSchedule][TIR] Patch for Data Type Inconsistency After Trace Reloading
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12035:
URL: https://github.com/apache/tvm/pull/12035#discussion_r916362468
##########
tests/python/unittest/test_meta_schedule_database.py:
##########
@@ -19,17 +19,25 @@
import os.path as osp
import tempfile
from typing import Callable
+import pytest
import tvm
+from tvm.meta_schedule.arg_info import TensorInfo
+from tvm.meta_schedule.builder.builder import BuilderInput
+from tvm.meta_schedule.builder.local_builder import LocalBuilder
+from tvm.meta_schedule.database.database import TuningRecord
+from tvm.meta_schedule.runner.local_runner import LocalRunner
+from tvm.meta_schedule.runner.runner import RunnerInput
Review Comment:
this is very verbose and we have decided previously to use `ms.builder.LocalBuilder` instead to make the logic clearer. please see the second point in this PR description: https://github.com/apache/tvm/pull/11622
--
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] junrushao1994 merged pull request #12035: [TIR] Avoid unnecessary dtype escalation in loop splitting
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged PR #12035:
URL: https://github.com/apache/tvm/pull/12035
--
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] zxybazh commented on pull request #12035: [MetaSchedule][TIR] Patch for Data Type Inconsistency After Trace Reloading
Posted by GitBox <gi...@apache.org>.
zxybazh commented on PR #12035:
URL: https://github.com/apache/tvm/pull/12035#issuecomment-1178500068
Fixed and all issues addressed! Thanks for the feedbacks.
--
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] junrushao1994 commented on pull request #12035: [TIR] Avoid unnecessary dtype escalation in loop splitting
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on PR #12035:
URL: https://github.com/apache/tvm/pull/12035#issuecomment-1179214307
Thanks @zxybazh for looking into that!
--
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] junrushao1994 commented on a diff in pull request #12035: [MetaSchedule][TIR] Patch for Data Type Inconsistency After Trace Reloading
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12035:
URL: https://github.com/apache/tvm/pull/12035#discussion_r916361951
##########
src/tir/schedule/concrete_schedule.cc:
##########
@@ -452,6 +452,9 @@ Array<LoopRV> ConcreteScheduleNode::Split(const LoopRV& loop_rv,
if (is_const_int(factor) && !is_positive_const(factor)) {
throw NonPositiveFactorError(state_->mod, factor.as<IntImmNode>()->value, i);
}
+ if (factor.dtype().bits() > loop->extent.dtype().bits()) {
+ factor = IntImm(loop->extent.dtype(), factor.as<IntImmNode>()->value);
+ }
Review Comment:
A `factor` isn't necessary a constant integer, so casting it using `factor.as<IntImmNode>()` is probably not the best here. Instead please use:
```python
if (factor.dtype().bits() > loop->extent.dtype().bits()) {
factor = tir::cast(loop->extent.dtype(), factor);
}
```
--
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