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 2020/07/13 06:24:49 UTC
[GitHub] [incubator-tvm] merrymercy opened a new pull request #6042: Add an option to limit the maximum extent of explicitly unrolled loop
merrymercy opened a new pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042
Explicitly unrolling a too long loop will make llvm code generation hang for a very long time.
Unrolling a long loop is also very uncommon. So this PR added an option to limit the maximum extent of explicitly unrolled loop.
The default maximum value is set to 32.
----------------------------------------------------------------
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] [incubator-tvm] comaniac commented on a change in pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042#discussion_r453778990
##########
File path: src/tir/transforms/unroll_loop.cc
##########
@@ -57,6 +58,9 @@ struct UnrollLoopConfigNode : public tvm::AttrsNode<UnrollLoopConfigNode> {
TVM_ATTR_FIELD(explicit_unroll)
.describe("Whether to explicitly unroll the loop instead of setting a pragma")
.set_default(true);
+ TVM_ATTR_FIELD(explicit_unroll_max_extent)
+ .describe("The maximum extent of a loop that can be unrolled explicitly (-1 for infinite)")
+ .set_default(32);
Review comment:
What's the performance impact of 16 on X86? Should we check the related TOPI templates, including X86 and ARM, for the performance impact?
----------------------------------------------------------------
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] [incubator-tvm] merrymercy commented on pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
merrymercy commented on pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042#issuecomment-693581680
Close this because this is not needed anymore.
----------------------------------------------------------------
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] [incubator-tvm] merrymercy commented on a change in pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
merrymercy commented on a change in pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042#discussion_r453785929
##########
File path: src/tir/transforms/unroll_loop.cc
##########
@@ -57,6 +58,9 @@ struct UnrollLoopConfigNode : public tvm::AttrsNode<UnrollLoopConfigNode> {
TVM_ATTR_FIELD(explicit_unroll)
.describe("Whether to explicitly unroll the loop instead of setting a pragma")
.set_default(true);
+ TVM_ATTR_FIELD(explicit_unroll_max_extent)
+ .describe("The maximum extent of a loop that can be unrolled explicitly (-1 for infinite)")
+ .set_default(32);
Review comment:
I think 32 is a very safe value.
This value is just to avoid the llvm hanging issue, not for code performance. 32 is enough to avoid almost all llvm issues. For the code performance, the schedule developers should choose a reasonable unroll factor by themself.
As a global configuration, this default value should be large enough to avoid performance regression. And I do not think 16 is safe enough.
##########
File path: src/tir/transforms/unroll_loop.cc
##########
@@ -57,6 +58,9 @@ struct UnrollLoopConfigNode : public tvm::AttrsNode<UnrollLoopConfigNode> {
TVM_ATTR_FIELD(explicit_unroll)
.describe("Whether to explicitly unroll the loop instead of setting a pragma")
.set_default(true);
+ TVM_ATTR_FIELD(explicit_unroll_max_extent)
+ .describe("The maximum extent of a loop that can be unrolled explicitly (-1 for infinite)")
+ .set_default(32);
Review comment:
I think 32 is a safe value.
This value is just to avoid the llvm hanging issue, not for code performance. 32 is enough to avoid almost all llvm issues. For the code performance, the schedule developers should choose a reasonable unroll factor by themself.
As a global configuration, this default value should be large enough to avoid performance regression. And I do not think 16 is safe enough.
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun commented on a change in pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042#discussion_r453809819
##########
File path: src/tir/transforms/unroll_loop.cc
##########
@@ -57,6 +58,9 @@ struct UnrollLoopConfigNode : public tvm::AttrsNode<UnrollLoopConfigNode> {
TVM_ATTR_FIELD(explicit_unroll)
.describe("Whether to explicitly unroll the loop instead of setting a pragma")
.set_default(true);
+ TVM_ATTR_FIELD(explicit_unroll_max_extent)
+ .describe("The maximum extent of a loop that can be unrolled explicitly (-1 for infinite)")
+ .set_default(32);
Review comment:
It's good to limit unrolling extent. However, would it be better to set default value as -1 so that user can still do arbitrarily unrolling by default and only limit it when necessary? Setting another default value can still potentially cause performance issue for existing tuned schedules.
----------------------------------------------------------------
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] [incubator-tvm] merrymercy commented on a change in pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
merrymercy commented on a change in pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042#discussion_r453785929
##########
File path: src/tir/transforms/unroll_loop.cc
##########
@@ -57,6 +58,9 @@ struct UnrollLoopConfigNode : public tvm::AttrsNode<UnrollLoopConfigNode> {
TVM_ATTR_FIELD(explicit_unroll)
.describe("Whether to explicitly unroll the loop instead of setting a pragma")
.set_default(true);
+ TVM_ATTR_FIELD(explicit_unroll_max_extent)
+ .describe("The maximum extent of a loop that can be unrolled explicitly (-1 for infinite)")
+ .set_default(32);
Review comment:
I think 32 is a very safe value. How do you know 32 is too large for ARM CPU?
This value is just to avoid the llvm hanging issue, not for code performance. (i.e. The schedule developers should choose a reasonable unroll factor by themself.)
As a global configuration, this default value should be large enough to avoid performance regression.
----------------------------------------------------------------
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] [incubator-tvm] merrymercy commented on a change in pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
merrymercy commented on a change in pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042#discussion_r453785929
##########
File path: src/tir/transforms/unroll_loop.cc
##########
@@ -57,6 +58,9 @@ struct UnrollLoopConfigNode : public tvm::AttrsNode<UnrollLoopConfigNode> {
TVM_ATTR_FIELD(explicit_unroll)
.describe("Whether to explicitly unroll the loop instead of setting a pragma")
.set_default(true);
+ TVM_ATTR_FIELD(explicit_unroll_max_extent)
+ .describe("The maximum extent of a loop that can be unrolled explicitly (-1 for infinite)")
+ .set_default(32);
Review comment:
I think 32 is a very safe value. How do you know 32 is too large for ARM CPU?
This value is to avoid the llvm hanging issue, not for performance.
The default value should be large enough to avoid performance regression.
----------------------------------------------------------------
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] [incubator-tvm] FrozenGene commented on a change in pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042#discussion_r453482210
##########
File path: src/tir/transforms/unroll_loop.cc
##########
@@ -57,6 +58,9 @@ struct UnrollLoopConfigNode : public tvm::AttrsNode<UnrollLoopConfigNode> {
TVM_ATTR_FIELD(explicit_unroll)
.describe("Whether to explicitly unroll the loop instead of setting a pragma")
.set_default(true);
+ TVM_ATTR_FIELD(explicit_unroll_max_extent)
+ .describe("The maximum extent of a loop that can be unrolled explicitly (-1 for infinite)")
+ .set_default(32);
Review comment:
How about change the default value to `16`? As like ARM CPU (especially arm 32 bits), 32 is still a little bigger for them. `16` should cover common hardwares.
----------------------------------------------------------------
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] [incubator-tvm] FrozenGene commented on pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042#issuecomment-657886398
I understand this pr ‘s purpose. I met 32 hangs for a bit long time on arm v7 cpu (Worse thing is single convolution op compile time is ok, but it is not well when to compile whole network). Previous fix is to limit the max_unroll to be 16. Everything works well.
----------------------------------------------------------------
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] [incubator-tvm] merrymercy closed pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
merrymercy closed pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042
----------------------------------------------------------------
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] [incubator-tvm] FrozenGene commented on a change in pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042#discussion_r453483351
##########
File path: src/tir/transforms/unroll_loop.cc
##########
@@ -165,6 +170,12 @@ class LoopUnroller : public StmtExprMutator {
// For loop must have a constant integer extent
CHECK_NE(value, -1) << "loop doesn't have a constant integer extent";
if (value == 0) return Evaluate(0);
+ if (explicit_unroll_max_extent_ > 0 && value > explicit_unroll_max_extent_ &&
+ explicit_unroll_) {
Review comment:
Suggest moving `explicit_unroll_` to the first condition. If it is `false`, we will trigger short circuit evaluation and will not evaluate other conditions.
----------------------------------------------------------------
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] [incubator-tvm] comaniac commented on a change in pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042#discussion_r453794016
##########
File path: src/tir/transforms/unroll_loop.cc
##########
@@ -57,6 +58,9 @@ struct UnrollLoopConfigNode : public tvm::AttrsNode<UnrollLoopConfigNode> {
TVM_ATTR_FIELD(explicit_unroll)
.describe("Whether to explicitly unroll the loop instead of setting a pragma")
.set_default(true);
+ TVM_ATTR_FIELD(explicit_unroll_max_extent)
+ .describe("The maximum extent of a loop that can be unrolled explicitly (-1 for infinite)")
+ .set_default(32);
Review comment:
Thanks for the explanation and I agree with your point. I just realized that this is a global config instead of the `unroll_explicit` pragma in the CUDA templates.
----------------------------------------------------------------
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] [incubator-tvm] kevinthesun commented on a change in pull request #6042: [TIR] Add an option to limit the maximum extent of explicitly unrolled loops
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #6042:
URL: https://github.com/apache/incubator-tvm/pull/6042#discussion_r453809819
##########
File path: src/tir/transforms/unroll_loop.cc
##########
@@ -57,6 +58,9 @@ struct UnrollLoopConfigNode : public tvm::AttrsNode<UnrollLoopConfigNode> {
TVM_ATTR_FIELD(explicit_unroll)
.describe("Whether to explicitly unroll the loop instead of setting a pragma")
.set_default(true);
+ TVM_ATTR_FIELD(explicit_unroll_max_extent)
+ .describe("The maximum extent of a loop that can be unrolled explicitly (-1 for infinite)")
+ .set_default(32);
Review comment:
It's good to limit unrolling extent. However, would it be better to set default value as -1 so that user can still do arbitrary unrolling by default and only limit it when necessary? Setting another default value can still potentially cause performance issue for existing tuned schedules.
----------------------------------------------------------------
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