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/01/07 23:32:01 UTC
[GitHub] [tvm] tkonolige opened a new pull request #9873: [TOPI] Add support for groupped conv3d
tkonolige opened a new pull request #9873:
URL: https://github.com/apache/tvm/pull/9873
Change conv3d to use generic conv implementation which supports grouped convolutions. Also, remove support for non-float16 tensorcore operations as they cause large degradation in accuracy. Generic conv now supports autoscheduler.
@mbrookhart @jwfromm
--
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] mbrookhart commented on a change in pull request #9873: [TOPI] Add support for groupped conv3d
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #9873:
URL: https://github.com/apache/tvm/pull/9873#discussion_r787169612
##########
File path: src/arith/rewrite_simplify.cc
##########
@@ -820,6 +820,10 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const FloorDivNode* op) {
TVM_TRY_REWRITE_IF(floordiv(x * y, y), x, CanProveGreaterEqual(y.Eval(), 0));
TVM_TRY_REWRITE_IF(floordiv(y * x, y), x, CanProveGreaterEqual(y.Eval(), 0));
+ if ((floordiv(x, y)).Match(ret) && analyzer_->CanProve(x.Eval() < y.Eval())) {
+ return 0;
+ }
+
Review comment:
Maybe need to a a pass test for this.
--
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] masahi merged pull request #9873: [TOPI] Add support for groupped conv3d
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #9873:
URL: https://github.com/apache/tvm/pull/9873
--
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] masahi commented on a change in pull request #9873: [TOPI] Add support for groupped conv3d
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #9873:
URL: https://github.com/apache/tvm/pull/9873#discussion_r787169459
##########
File path: python/tvm/relay/op/strategy/cuda.py
##########
@@ -641,7 +642,7 @@ def conv3d_strategy_cuda(attrs, inputs, out_type, target):
(N % 16 == 0 and CI % 16 == 0 and CO % 16 == 0)
or (N % 8 == 0 and CI % 16 == 0 and CO % 32 == 0)
or (N % 32 == 0 and CI % 16 == 0 and CO % 8 == 0)
- ):
+ ) and out_type == "float16":
Review comment:
How about making this change to conv2d as well?. Perhaps better to send a separate PR for disabling fp32 tensorcore use since that would be a breaking change and potentially controversial.
--
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] tkonolige commented on a change in pull request #9873: [TOPI] Add support for groupped conv3d
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #9873:
URL: https://github.com/apache/tvm/pull/9873#discussion_r795907495
##########
File path: python/tvm/relay/op/strategy/cuda.py
##########
@@ -641,7 +642,7 @@ def conv3d_strategy_cuda(attrs, inputs, out_type, target):
(N % 16 == 0 and CI % 16 == 0 and CO % 16 == 0)
or (N % 8 == 0 and CI % 16 == 0 and CO % 32 == 0)
or (N % 32 == 0 and CI % 16 == 0 and CO % 8 == 0)
- ):
+ ) and out_type == "float16":
Review comment:
This was causing an issue with the new schedule, so I disabled it.
##########
File path: python/tvm/relay/op/strategy/cuda.py
##########
@@ -641,7 +642,7 @@ def conv3d_strategy_cuda(attrs, inputs, out_type, target):
(N % 16 == 0 and CI % 16 == 0 and CO % 16 == 0)
or (N % 8 == 0 and CI % 16 == 0 and CO % 32 == 0)
or (N % 32 == 0 and CI % 16 == 0 and CO % 8 == 0)
- ):
+ ) and out_type == "float16":
Review comment:
This was causing an issue with the new compute, so I disabled it.
--
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] masahi edited a comment on pull request #9873: [TOPI] Add support for groupped conv3d
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #9873:
URL: https://github.com/apache/tvm/pull/9873#issuecomment-1015850768
> Also, remove support for non-float16 tensorcore operations as they cause large degradation in accuracy
Yeah, I also noticed this implicit downcasting before, I'm in favor of this change. Users can always explicitly cast their inputs to fp16. The original code was added before we have the fp16 conversion pass.
This may break existing flows if some users depend on this behavior, which is also the default in cudnn/cublas. Personally I don't mind it, but if that's concerning we can always add a workaround to allow the old 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] masahi commented on a change in pull request #9873: [TOPI] Add support for groupped conv3d
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #9873:
URL: https://github.com/apache/tvm/pull/9873#discussion_r787169459
##########
File path: python/tvm/relay/op/strategy/cuda.py
##########
@@ -641,7 +642,7 @@ def conv3d_strategy_cuda(attrs, inputs, out_type, target):
(N % 16 == 0 and CI % 16 == 0 and CO % 16 == 0)
or (N % 8 == 0 and CI % 16 == 0 and CO % 32 == 0)
or (N % 32 == 0 and CI % 16 == 0 and CO % 8 == 0)
- ):
+ ) and out_type == "float16":
Review comment:
How about making this change to conv2d as well?. Perhaps better to send a separate PR for disabling fp32 tensorcore use since that would be a breaking change and potentially controversial.
I remember the dense op rejects fp32 inputs for tensorcore too, so it is better in terms of consistency too.
--
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] masahi commented on pull request #9873: [TOPI] Add support for groupped conv3d
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #9873:
URL: https://github.com/apache/tvm/pull/9873#issuecomment-1015850768
> Also, remove support for non-float16 tensorcore operations as they cause large degradation in accuracy
Yeah, I also noticed this implicit downcasting before, I'm in favor of this change. Users can always explicitly cast their inputs to fp16.
This may break existing flows if some users depend on this behavior, which is also the default in cudnn/cublas. Personally I don't mind it, but if that's concerning we can always add a workaround to allow the old 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] tkonolige commented on a change in pull request #9873: [TOPI] Add support for groupped conv3d
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #9873:
URL: https://github.com/apache/tvm/pull/9873#discussion_r787217426
##########
File path: src/arith/rewrite_simplify.cc
##########
@@ -820,6 +820,10 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const FloorDivNode* op) {
TVM_TRY_REWRITE_IF(floordiv(x * y, y), x, CanProveGreaterEqual(y.Eval(), 0));
TVM_TRY_REWRITE_IF(floordiv(y * x, y), x, CanProveGreaterEqual(y.Eval(), 0));
+ if ((floordiv(x, y)).Match(ret) && analyzer_->CanProve(x.Eval() < y.Eval())) {
+ return 0;
+ }
+
Review comment:
This is needed so that the simplifier here: https://github.com/apache/tvm/pull/9873/files#diff-04a1aab966320b6f63c390cc8b79f79b543a1a7a3f560ea3a97d18c2bb041a0aR837-R839 will remove the division by groups. Without this, simplification get stuck simplifying `ff // (num_filter // groups) * (in_channel // groups) + rc` to `ff // num_filter * in_channel + rc`. It can't prove that `ff // num_filter = 0`.
--
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] tkonolige commented on pull request #9873: [TOPI] Add support for groupped conv3d
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #9873:
URL: https://github.com/apache/tvm/pull/9873#issuecomment-1043138810
@masahi @mbrookhart I've finally got this branch green. Could you re-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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbrookhart commented on a change in pull request #9873: [TOPI] Add support for groupped conv3d
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #9873:
URL: https://github.com/apache/tvm/pull/9873#discussion_r787157315
##########
File path: src/target/source/codegen_cuda.cc
##########
@@ -1051,18 +1051,27 @@ void CodeGenCUDA::PrintWmmaScope(const std::string& scope, DataType t, const Var
}
}
+int stoi(const std::string& str) {
+ try {
+ return std::stoi(str);
+ } catch (std::invalid_argument& e) {
+ LOG(FATAL) << "Cannot convert \"" << str << "\" to int";
+ throw;
Review comment:
I don't think you need this throw, LOG(FATAL) throws under the hood.
##########
File path: src/arith/rewrite_simplify.cc
##########
@@ -820,6 +820,10 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const FloorDivNode* op) {
TVM_TRY_REWRITE_IF(floordiv(x * y, y), x, CanProveGreaterEqual(y.Eval(), 0));
TVM_TRY_REWRITE_IF(floordiv(y * x, y), x, CanProveGreaterEqual(y.Eval(), 0));
+ if ((floordiv(x, y)).Match(ret) && analyzer_->CanProve(x.Eval() < y.Eval())) {
+ return 0;
+ }
+
Review comment:
I'm not sure what this is for?
--
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] tkonolige commented on a change in pull request #9873: [TOPI] Add support for groupped conv3d
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #9873:
URL: https://github.com/apache/tvm/pull/9873#discussion_r787246770
##########
File path: src/target/source/codegen_cuda.cc
##########
@@ -1051,18 +1051,27 @@ void CodeGenCUDA::PrintWmmaScope(const std::string& scope, DataType t, const Var
}
}
+int stoi(const std::string& str) {
+ try {
+ return std::stoi(str);
+ } catch (std::invalid_argument& e) {
+ LOG(FATAL) << "Cannot convert \"" << str << "\" to int";
+ throw;
Review comment:
Somehow the compiler can't see through LOG(FATAL). I think it is because it actually throws in the destructor.
--
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