[GitHub] [incubator-tvm] FrozenGene commented on issue #5148: [Strategy][ARM CPU] Remove contrib spatial pack schedule of depthwise convolution

[GitBox]

FrozenGene commented on issue #5148: [Strategy][ARM CPU] Remove contrib spatial 
pack schedule of depthwise convolution
URL: https://github.com/apache/incubator-tvm/pull/5148#issuecomment-604232873
 
 
   Thanks @icemelon9 @comaniac for reviewing and giving advices.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[incubator-tvm] branch master updated: [Strategy][ARM CPU] Remove contrib spatial pack schedule of depthwise convolution (#5148)

[zhaowu]

This is an automated email from the ASF dual-hosted git repository.

zhaowu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-tvm.git


The following commit(s) were added to refs/heads/master by this push:
 new 314f31b  [Strategy][ARM CPU] Remove contrib spatial pack schedule of 
depthwise convolution (#5148)
314f31b is described below

commit 314f31b0b4e2dbc934d74afe90bff1ef3ac40b17
Author: Zhao Wu 
AuthorDate: Thu Mar 26 13:10:00 2020 +0800

[Strategy][ARM CPU] Remove contrib spatial pack schedule of depthwise 
convolution (#5148)

* [Strategy][ARM CPU] Low the plevel of contrib spatial pack of depthwise 
convolution

* address comments
---
 python/tvm/relay/op/strategy/arm_cpu.py  | 15 ++-
 topi/python/topi/arm_cpu/depthwise_conv2d.py |  5 +
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/python/tvm/relay/op/strategy/arm_cpu.py 
b/python/tvm/relay/op/strategy/arm_cpu.py
index 87e48dc..a803bb6 100644
--- a/python/tvm/relay/op/strategy/arm_cpu.py
+++ b/python/tvm/relay/op/strategy/arm_cpu.py
@@ -105,11 +105,16 @@ def conv2d_strategy_arm_cpu(attrs, inputs, out_type, 
target):
 wrap_compute_conv2d(topi.arm_cpu.depthwise_conv2d_nchw),
 
wrap_topi_schedule(topi.arm_cpu.schedule_depthwise_conv2d_nchw),
 name="depthwise_conv2d_nchw.arm_cpu")
-strategy.add_implementation(
-
wrap_compute_conv2d(topi.arm_cpu.depthwise_conv2d_nchw_spatial_pack),
-
wrap_topi_schedule(topi.arm_cpu.schedule_depthwise_conv2d_nchw_spatial_pack),
-name="depthwise_conv2d_nchw_spatial_pack.arm_cpu",
-plevel=15)
+# TODO:
+# This schedule has incorrect result on some hardware platforms 
(like NV Jetson TX2)
+# Let us comment it out but not remove.
+# see discussion:
+# 
https://discuss.tvm.ai/t/autotuner-incorrect-result-after-tuning-mobilenetv2-on-arm-cpu/6088
+# strategy.add_implementation(
+# 
wrap_compute_conv2d(topi.arm_cpu.depthwise_conv2d_nchw_spatial_pack),
+# 
wrap_topi_schedule(topi.arm_cpu.schedule_depthwise_conv2d_nchw_spatial_pack),
+# name="depthwise_conv2d_nchw_spatial_pack.arm_cpu",
+# plevel=15)
 elif layout == "NHWC":
 assert kernel_layout == "HWOI"
 logger.warning("depthwise_conv2d with layout NHWC is not optimized 
for arm cpu.")
diff --git a/topi/python/topi/arm_cpu/depthwise_conv2d.py 
b/topi/python/topi/arm_cpu/depthwise_conv2d.py
index 5214972..802b3df 100644
--- a/topi/python/topi/arm_cpu/depthwise_conv2d.py
+++ b/topi/python/topi/arm_cpu/depthwise_conv2d.py
@@ -139,6 +139,11 @@ def schedule_depthwise_conv2d_nchw(cfg, outs):
 return s
 
 
+# TODO:
+# This schedule has incorrect result on some hardware platforms (like NV 
Jetson TX2)
+# Let us comment it out but not remove.
+# see discussion:
+# 
https://discuss.tvm.ai/t/autotuner-incorrect-result-after-tuning-mobilenetv2-on-arm-cpu/6088
 @autotvm.register_topi_compute("depthwise_conv2d_nchw_spatial_pack.arm_cpu")
 def depthwise_conv2d_nchw_spatial_pack(cfg, data, kernel, strides, padding, 
dilation, out_dtype):
 """TOPI compute callback for depthwise_conv2d nchw

[GitHub] [incubator-tvm] FrozenGene merged pull request #5148: [Strategy][ARM CPU] Remove contrib spatial pack schedule of depthwise convolution

[GitBox]

FrozenGene merged pull request #5148: [Strategy][ARM CPU] Remove contrib 
spatial pack schedule of depthwise convolution
URL: https://github.com/apache/incubator-tvm/pull/5148
 
 
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] icemelon9 commented on issue #5131: [Relay][OP] Register topi schedule for Relay fast_exp and fast_tanh

[GitBox]

icemelon9 commented on issue #5131: [Relay][OP] Register topi schedule for 
Relay fast_exp and fast_tanh
URL: https://github.com/apache/incubator-tvm/pull/5131#issuecomment-604206058
 
 
   Yes, could you also add fast_tanh in the test_fastmath?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] selo1412 edited a comment on issue #5131: [Relay][OP] Register topi schedule for Relay fast_exp and fast_tanh

[GitBox]

selo1412 edited a comment on issue #5131: [Relay][OP] Register topi schedule 
for Relay fast_exp and fast_tanh
URL: https://github.com/apache/incubator-tvm/pull/5131#issuecomment-604183798
 
 
   @comaniac @icemelon9 
   Thanks for your help.
   And, what should I do next?
   
   By the way, I find that 
   tvm/topi/tests/python/test_topi_math.py
   maybe, test_apply(fast_tanh) is missing in test_fastmath(). 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] FrozenGene commented on issue #5146: Handle empty LLVMModule in GetFunction

[GitBox]

FrozenGene commented on issue #5146: Handle empty LLVMModule in GetFunction
URL: https://github.com/apache/incubator-tvm/pull/5146#issuecomment-604187370
 
 
   Thanks @kumasento for fixing it. Thanks @trevor-m @zhiics for verifying and 
reviewing. 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] FrozenGene merged pull request #5146: Handle empty LLVMModule in GetFunction

[GitBox]

FrozenGene merged pull request #5146: Handle empty LLVMModule in GetFunction
URL: https://github.com/apache/incubator-tvm/pull/5146
 
 
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[incubator-tvm] branch master updated: Handle empty LLVMModule in GetFunction (#5146)

[zhaowu]

This is an automated email from the ASF dual-hosted git repository.

zhaowu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-tvm.git


The following commit(s) were added to refs/heads/master by this push:
 new 08b38be  Handle empty LLVMModule in GetFunction (#5146)
08b38be is described below

commit 08b38be95f4eb4e9c6c1c4d41a5219b9ba689bf1
Author: Ruizhe Zhao 
AuthorDate: Thu Mar 26 02:11:29 2020 +

Handle empty LLVMModule in GetFunction (#5146)
---
 src/target/llvm/llvm_module.cc | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/target/llvm/llvm_module.cc b/src/target/llvm/llvm_module.cc
index 6cf9f11..3f508a5 100644
--- a/src/target/llvm/llvm_module.cc
+++ b/src/target/llvm/llvm_module.cc
@@ -71,6 +71,10 @@ class LLVMModuleNode final : public runtime::ModuleNode {
   });
 }
 if (ee_ == nullptr) LazyInitJIT();
+
+// This LLVMModule is empty and no function can be retrieved.
+if (entry_func_.empty()) return nullptr;
+
 std::lock_guard lock(mutex_);
 const std::string& fname = (name == runtime::symbol::tvm_module_main ?
 entry_func_ : name);
@@ -318,6 +322,10 @@ class LLVMModuleNode final : public runtime::ModuleNode {
 << "Failed to initialize jit engine for " << mptr_->getTargetTriple();
 ee_->runStaticConstructorsDestructors(false);
 // setup context address.
+// we will skip context setup if this LLVMModule is empty.
+if (GetGlobalAddr(runtime::symbol::tvm_module_main) == 0)
+  return;
+
 entry_func_ =
 reinterpret_cast(GetGlobalAddr(runtime::symbol::tvm_module_main));
 if (void** ctx_addr = reinterpret_cast(
@@ -329,7 +337,7 @@ class LLVMModuleNode final : public runtime::ModuleNode {
   });
   }
   // Get global address from execution engine.
-  uint64_t GetGlobalAddr(const std::string& name) {
+  uint64_t GetGlobalAddr(const std::string& name) const {
 // first verifies if GV exists.
 if (mptr_->getGlobalVariable(name) != nullptr) {
   return ee_->getGlobalValueAddress(name);
@@ -337,7 +345,7 @@ class LLVMModuleNode final : public runtime::ModuleNode {
   return 0;
 }
   }
-  uint64_t GetFunctionAddr(const std::string& name) {
+  uint64_t GetFunctionAddr(const std::string& name) const {
 // first verifies if GV exists.
 if (mptr_->getFunction(name) != nullptr) {
   return ee_->getFunctionAddress(name);

[GitHub] [incubator-tvm] FrozenGene commented on issue #5148: [Strategy][ARM CPU] Remove contrib spatial pack schedule of depthwise convolution

[GitBox]

FrozenGene commented on issue #5148: [Strategy][ARM CPU] Remove contrib spatial 
pack schedule of depthwise convolution
URL: https://github.com/apache/incubator-tvm/pull/5148#issuecomment-604186487
 
 
   > Yes, let's comment out this implementation temporarily. Could you add a 
Todo with pointer to the issue in the discussion to both arm cpu strategy and 
the topi file?
   
   Sure. I have added comments as your advice. @comaniac @icemelon9 please have 
a look again


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5092: [TIR][PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on a change in pull request #5092: [TIR][PASS] dtype rewrite 
for indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r398275074
 
 

 ##
 File path: src/tir/pass/narrow_datatype.cc
 ##
 @@ -0,0 +1,373 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file narrow_datatype.cc
+ * \brief narrow the datatype of indexing vars
+ */
+
+#include 
+#include 
+#include "../../arith/ir_mutator_with_analyzer.h"
+#include "../../arith/ir_visitor_with_analyzer.h"
+
+namespace tvm {
+namespace tir {
+
+// This pass narrows indexing expressions (like StoreNode::Index)
+// that trivially fit into i32 to i32. Considering that i32 indices
+// may be more efficient on some backends (while i64 may be more
+// efficient on others, like llvm), we may want this pass when i32
+// indices are more efficient.
+//
+// For Var v, we determine its dtype by examining all the PrimExpr
+// that contains v, denoted by E = {e_0 = v, e_1, e_2, ..., e_k}.
+// If all expressions in E fit into i32, then we think v can be narrowed
+// to i32.
+//
+// To make an indexing expression i32, we must make sure that every
+// component of that expression is of dtype i32. So besides Var, we
+// rewrite the following inside an indexing expression
+// - Var
+// - IntImm
+// - Cast
+//
+// Algorithm:
+// - Use DataTypeVisitor to determine whether a Var can be narrowed or not.
+// - Use DataTypeRewritter to rewrite the components of an indexing expression.
+
+using arith::Analyzer;
+using arith::IRMutatorWithAnalyzer;
+using arith::ConstIntBound;
+
+class DataTypeVisitor final : public StmtExprVisitor {
+ public:
+  explicit DataTypeVisitor(int target_bits)
+: bits_(target_bits), target_bits_(target_bits) {}
+
+  void VisitExpr(const PrimExpr& e) {
+if (e.dtype().is_int()) {
+  int bits = max_bits_;
+  ConstIntBound bound = analyzer_.const_int_bound(e);
+  int64_t ubound = Downcast(max_value(DataType::Int(target_bits_)))->value;
+  int64_t lbound = Downcast(min_value(DataType::Int(target_bits_)))->value;
+  if (e.dtype().bits() <= target_bits_ ||
 
 Review comment:
   To clarify, this code seems to indicate that we can rewrite lower bits into 
higher ones, and I think we do not need this 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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] selo1412 commented on issue #5131: [Relay][OP] Register topi schedule for Relay fast_exp and fast_tanh

[GitBox]

selo1412 commented on issue #5131: [Relay][OP] Register topi schedule for Relay 
fast_exp and fast_tanh
URL: https://github.com/apache/incubator-tvm/pull/5131#issuecomment-604183798
 
 
   @comaniac @icemelon9 
   Thanks for your help.
   And, what should I do next?
   
   By the way, I find that 
   tvm/topi/tests/python/test_topi_math.py
   Maybe, test_apply(fast_tanh) is missing in test_fastmath(). 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] liangfu commented on issue #5060: [uTVM][Runtime] Deprecate uTVM Standalone Runtime

[GitBox]

liangfu commented on issue #5060: [uTVM][Runtime] Deprecate uTVM Standalone 
Runtime
URL: https://github.com/apache/incubator-tvm/issues/5060#issuecomment-604163218
 
 
   Sure, as this is definitely the direction we should follow, I can do that. 
And maybe we need a separate PR for the arena allocator feature.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] liangfu commented on a change in pull request #5147: [RUNTIME]crt error handling

[GitBox]

liangfu commented on a change in pull request #5147: [RUNTIME]crt error handling
URL: https://github.com/apache/incubator-tvm/pull/5147#discussion_r398233351
 
 

 ##
 File path: src/runtime/crt/graph_runtime.c
 ##
 @@ -38,25 +38,28 @@ uint32_t Shape_Accumulate(int64_t * shape, uint32_t ndim) {
 }
 
 int NodeEntry_Load(TVMGraphRuntimeNodeEntry * entry, JSONReader * reader) {
-  int status = 0;
+  int status = -1;
   reader->BeginArray(reader);
   if (!(reader->NextArrayItem(reader))) {
 fprintf(stderr, "invalid json format: failed to parse `node_id`\n");
+return status;
 
 Review comment:
   MISRA-C requires having only one return statement in a function.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] liangfu commented on a change in pull request #5147: [RUNTIME]crt error handling

[GitBox]

liangfu commented on a change in pull request #5147: [RUNTIME]crt error handling
URL: https://github.com/apache/incubator-tvm/pull/5147#discussion_r398234478
 
 

 ##
 File path: src/runtime/crt/graph_runtime.c
 ##
 @@ -38,25 +38,28 @@ uint32_t Shape_Accumulate(int64_t * shape, uint32_t ndim) {
 }
 
 int NodeEntry_Load(TVMGraphRuntimeNodeEntry * entry, JSONReader * reader) {
-  int status = 0;
+  int status = -1;
 
 Review comment:
   The default is intended to be success, and set status to error when we are 
in trouble. Subsequently, return status at the end of the function.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] vinx13 closed issue #5145: [BUG][TUTORIAL] Tutorial for quantization need update

[GitBox]

vinx13 closed issue #5145:  [BUG][TUTORIAL] Tutorial for quantization need 
update
URL: https://github.com/apache/incubator-tvm/issues/5145
 
 
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] vinx13 merged pull request #5150: [Tutorial][Quantization] Fix incorrect name of calibration mode

[GitBox]

vinx13 merged pull request #5150: [Tutorial][Quantization] Fix incorrect name 
of calibration mode
URL: https://github.com/apache/incubator-tvm/pull/5150
 
 
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[incubator-tvm] branch master updated: [Tutorial][Quantization] Fix incorrect name of calibration mode (#5150)

[wuwei]

This is an automated email from the ASF dual-hosted git repository.

wuwei pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-tvm.git


The following commit(s) were added to refs/heads/master by this push:
 new a1d6fc9  [Tutorial][Quantization] Fix incorrect name of calibration 
mode (#5150)
a1d6fc9 is described below

commit a1d6fc95042c9df9dd07fe537a40f3bedc62ef72
Author: Wuwei Lin 
AuthorDate: Wed Mar 25 18:27:17 2020 -0400

[Tutorial][Quantization] Fix incorrect name of calibration mode (#5150)
---
 tutorials/frontend/deploy_quantized.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tutorials/frontend/deploy_quantized.py 
b/tutorials/frontend/deploy_quantized.py
index 5af9fc9..2586318 100644
--- a/tutorials/frontend/deploy_quantized.py
+++ b/tutorials/frontend/deploy_quantized.py
@@ -132,7 +132,7 @@ def quantize(mod, params, data_aware):
 with relay.quantize.qconfig(calibrate_mode='kl_divergence', 
weight_scale='max'):
 mod = relay.quantize.quantize(mod, params, 
dataset=calibrate_dataset())
 else:
-with relay.quantize.qconfig(calibrate_mode='global', global_scale=8.0):
+with relay.quantize.qconfig(calibrate_mode='global_scale', 
global_scale=8.0):
 mod = relay.quantize.quantize(mod, params)
 return mod
 

[incubator-tvm] branch master updated (3aabbd9 -> 0c38b91)

[zhic]

This is an automated email from the ASF dual-hosted git repository.

zhic pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-tvm.git.


from 3aabbd9  [Torch] Add support for max_pool1d (#5142)
 add 0c38b91  Duplicate likely nodes added when loop axis split unevenly 
(#5084)

No new revisions were added by this update.

Summary of changes:
 src/te/schedule/message_passing.cc   | 10 +-
 tests/python/unittest/test_te_build_lower.py | 13 +
 2 files changed, 22 insertions(+), 1 deletion(-)

[GitHub] [incubator-tvm] zhiics commented on issue #5084: Duplicate likely nodes added when loop axis split unevenly

[GitBox]

zhiics commented on issue #5084: Duplicate likely nodes added when loop axis 
split unevenly
URL: https://github.com/apache/incubator-tvm/pull/5084#issuecomment-604104881
 
 
   Thanks @ANSHUMAN87 @MarisaKirisame @yzhliu 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics merged pull request #5084: Duplicate likely nodes added when loop axis split unevenly

[GitBox]

zhiics merged pull request #5084: Duplicate likely nodes added when loop axis 
split unevenly
URL: https://github.com/apache/incubator-tvm/pull/5084
 
 
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] masahi edited a comment on issue #5149: [WIP][Relay][Frontend] Chainer frontend introduced

[GitBox]

masahi edited a comment on issue #5149: [WIP][Relay][Frontend] Chainer frontend 
introduced
URL: https://github.com/apache/incubator-tvm/pull/5149#issuecomment-604095365
 
 
   You should first send a RFC on why you want to add this frontend. I don't 
think it is a good idea for the following reasons:
   
   * Chainer is now in maintenance mode
   * We already have ONNX and PyTorch frontend. Existing Chainer users are 
being encouraged to switch to PyTorch, so they can use either frontend
   * Additional frontend brings more development and maintenance burden 
(including reviews), and makes CI even slower 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] yzhliu commented on issue #5121: [TE] reverse-mode autodiff without any optimization

[GitBox]

yzhliu commented on issue #5121: [TE] reverse-mode autodiff without any 
optimization
URL: https://github.com/apache/incubator-tvm/pull/5121#issuecomment-604099502
 
 
   CI's green. @tqchen @hzfan check if there's anything needs to be addressed.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] masahi commented on issue #5149: [WIP][Relay][Frontend] Chainer frontend introduced

[GitBox]

masahi commented on issue #5149: [WIP][Relay][Frontend] Chainer frontend 
introduced
URL: https://github.com/apache/incubator-tvm/pull/5149#issuecomment-604095365
 
 
   You should first send a RFC on why you want to add this frontend. I don't 
think it is a good idea for the following reasons:
   
   * Chainer is now in maintenance mode
   * We already have ONNX and PyTorch frontend. Existing Chainer users are 
being encouraged to switch to PyTorch, so they can use either frontend


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] jwfromm commented on a change in pull request #5099: [TOPI][Tensor Core] Conv2d and Dense ops support on Tensor Core

[GitBox]

jwfromm commented on a change in pull request #5099: [TOPI][Tensor Core] Conv2d 
and Dense ops support on Tensor Core
URL: https://github.com/apache/incubator-tvm/pull/5099#discussion_r395912368
 
 

 ##
 File path: topi/python/topi/cuda/conv2d_nhwc_tensorcore.py
 ##
 @@ -0,0 +1,361 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=invalid-name, too-many-locals, too-many-arguments
+# pylint: disable=too-many-statements, unused-argument
+"""Tensorcore template for cuda backend"""
+import numpy as np
+import tvm
+from tvm import te
+from tvm import autotvm
+from ..util import get_const_tuple, traverse_inline, simplify
+from ..nn.pad import pad
+from ..nn.util import get_pad_tuple
+from .tensor_intrin import intrin_wmma_load_matrix_A
+from .tensor_intrin import intrin_wmma_load_matrix_W
+from .tensor_intrin import intrin_wmma_store_matrix
+
+
+def intrin_wmma_gemm(strides_A, strides_W, strides_Conv, shape, out_dtype):
+"""Intrin for wmma fill_fragment and mma_sync"""
+wmma_m, wmma_n, wmma_k = shape
+A = te.placeholder((wmma_m, 1, 1, wmma_k), name='A', dtype='float16')
+B = te.placeholder((wmma_k, wmma_n), name='B', dtype='float16')
+k = te.reduce_axis((0, wmma_k), name="k")
+C = te.compute((wmma_m, 1, 1, wmma_n),
+   lambda ii, t0, t1, jj:
+   te.sum(A[ii, t0, t1, k].astype(out_dtype) * \
+  B[k, jj].astype(out_dtype), axis=k),
+   name='C')
+BA = tvm.tir.decl_buffer(A.shape, A.dtype, name='BA',
+ scope='wmma.matrix_a', data_alignment=32,
+ offset_factor=8, strides=strides_A)
+BB = tvm.tir.decl_buffer(B.shape, B.dtype, name='BB',
+ scope='wmma.matrix_b', data_alignment=32,
+ offset_factor=8, strides=strides_W)
+BC = tvm.tir.decl_buffer(C.shape, C.dtype, name='BC',
+ scope='wmma.accumulator', data_alignment=32,
+ offset_factor=8, strides=strides_Conv)
+
+def intrin_func(ins, outs):
+BA, BB = ins
+BC, = outs
+
+def warp_idnex(offset, row, col):
+row = row * col
+return offset // row + offset % row // col
+
+warp_index_A = warp_idnex(BA.elem_offset, wmma_m, wmma_k)
+warp_index_B = warp_idnex(BB.elem_offset, wmma_k, wmma_n)
+warp_index_C = warp_idnex(BC.elem_offset, wmma_m, wmma_n)
+
+def init():
+ib = tvm.tir.ir_builder.create()
+ib.emit(
+tvm.tir.call_intrin('handle', 'tvm_fill_fragment', BC.data, 
wmma_m, wmma_n, wmma_k,
+warp_index_C, 0.0))
+return ib.get()
+
+def update():
+ib = tvm.tir.ir_builder.create()
+ib.emit(tvm.tir.call_intrin('handle', 'tvm_mma_sync',
+BC.data, warp_index_C,
+BA.data, warp_index_A,
+BB.data, warp_index_B,
+BC.data, warp_index_C))
+return ib.get()
+
+return update(), init(), update()
+
+return te.decl_tensor_intrin(C.op, intrin_func, binds={A: BA, B: BB, C: 
BC})
+
+
+def nhwc_tensorcore_cuda(cfg, Input, Filter, stride, padding, dilation, 
out_dtype):
+"""Compute declaration for tensorcore"""
+assert isinstance(stride, int) or len(stride) == 2
+assert isinstance(dilation, int) or len(dilation) == 2
+
+if isinstance(stride, int):
+stride_h = stride_w = stride
+else:
+stride_h, stride_w = stride
+
+if isinstance(dilation, int):
+dilation_h = dilation_w = dilation
+else:
+dilation_h, dilation_w = dilation
+
+batch, in_height, in_width, in_channel = Input.shape
+kernel_h, kernel_w, _, num_filter = Filter.shape
+# compute the output shape
+dilated_kernel_h = (kernel_h - 1) * dilation_h + 1
+dilated_kernel_w = (kernel_w - 1) * dilation_w + 1
+pad_top, pad_left, pad_down, pad_right = get_pad_tuple(
+padding, (dilated_kernel_h, dilated_kernel_w))
+out_channel = num_filter

[GitHub] [incubator-tvm] Robeast commented on issue #5145: [BUG][TUTORIAL] Tutorial for quantization need update

[GitBox]

Robeast commented on issue #5145:  [BUG][TUTORIAL] Tutorial for quantization 
need update
URL: https://github.com/apache/incubator-tvm/issues/5145#issuecomment-604081129
 
 
   Thank you for your understanding... And thank you very much for @vinx13 for 
fixing!


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] trevor-m commented on issue #5146: Handle empty LLVMModule in GetFunction

[GitBox]

trevor-m commented on issue #5146: Handle empty LLVMModule in GetFunction
URL: https://github.com/apache/incubator-tvm/pull/5146#issuecomment-604059024
 
 
   Thanks @kumasento ! I have confirmed that this fixes my issue with external 
codegen.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on issue #5145: [BUG][TUTORIAL] Tutorial for quantization need update

[GitBox]

tqchen commented on issue #5145:  [BUG][TUTORIAL] Tutorial for quantization 
need update
URL: https://github.com/apache/incubator-tvm/issues/5145#issuecomment-604050409
 
 
   no problem, thanks @Robeast for reporting the problem :)


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] vinx13 opened a new pull request #5150: [Tutorial][Quantization] Fix incorrect name of calibration mode

[GitBox]

vinx13 opened a new pull request #5150: [Tutorial][Quantization] Fix incorrect 
name of calibration mode
URL: https://github.com/apache/incubator-tvm/pull/5150
 
 
   Fix #5145 
   
   @Robeast @tqchen 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] Robeast commented on issue #5145: [BUG][TUTORIAL] Tutorial for quantization need update

[GitBox]

Robeast commented on issue #5145:  [BUG][TUTORIAL] Tutorial for quantization 
need update
URL: https://github.com/apache/incubator-tvm/issues/5145#issuecomment-604048230
 
 
   @tqchen I'm extremely ashamed to say this, but due to company policy I'm not 
allowed to contribute to anything other than company code bases. Especially 
shameful since it's only 1 string change... :( I'm really sorry.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] mbaret commented on a change in pull request #5106: [Relay][MergeComposite] Support TupleGetItem in body of pattern

[GitBox]

mbaret commented on a change in pull request #5106: [Relay][MergeComposite] 
Support TupleGetItem in body of pattern
URL: https://github.com/apache/incubator-tvm/pull/5106#discussion_r398110062
 
 

 ##
 File path: src/relay/transforms/merge_composite.cc
 ##
 @@ -66,6 +66,31 @@ class MergeCompositeWrapper : public ExprMutator {
 return root;
   }
 
+  Expr ExtractPattern(const TupleGetItem& pattern, const Expr& root,
+  Map>* var_map, Map* call_map) {
+if (!root->IsInstance()) {
+  return Expr();
+}
+auto root_node = Downcast(root);
+if (pattern->index != root_node->index) {
+  return Expr();
+}
+if (pattern->tuple->IsInstance() &&
+root_node->tuple->IsInstance()) {
+  Expr new_arg;
+  if (call_map->find(pattern->tuple) != call_map->end()) {
+new_arg = (*call_map)[pattern->tuple];
+  } else {
+new_arg = ExtractPattern(Downcast(pattern->tuple),
+ Downcast(root_node->tuple),
+ var_map, call_map);
+call_map->Set(pattern->tuple, new_arg);
 
 Review comment:
   Alright, we'll leave it for now then (I'll probably change it in a later PR).


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] mbaret commented on a change in pull request #5106: [Relay][MergeComposite] Support TupleGetItem in body of pattern

[GitBox]

mbaret commented on a change in pull request #5106: [Relay][MergeComposite] 
Support TupleGetItem in body of pattern
URL: https://github.com/apache/incubator-tvm/pull/5106#discussion_r398109720
 
 

 ##
 File path: src/relay/transforms/merge_composite.cc
 ##
 @@ -66,6 +66,31 @@ class MergeCompositeWrapper : public ExprMutator {
 return root;
   }
 
+  Expr ExtractPattern(const TupleGetItem& pattern, const Expr& root,
+  Map>* var_map, Map* call_map) {
+if (!root->IsInstance()) {
+  return Expr();
+}
+auto root_node = Downcast(root);
+if (pattern->index != root_node->index) {
+  return Expr();
+}
+if (pattern->tuple->IsInstance() &&
+root_node->tuple->IsInstance()) {
+  Expr new_arg;
+  if (call_map->find(pattern->tuple) != call_map->end()) {
+new_arg = (*call_map)[pattern->tuple];
+  } else {
+new_arg = ExtractPattern(Downcast(pattern->tuple),
 
 Review comment:
   Ah, I get it now, 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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] trevor-m commented on a change in pull request #5106: [Relay][MergeComposite] Support TupleGetItem in body of pattern

[GitBox]

trevor-m commented on a change in pull request #5106: [Relay][MergeComposite] 
Support TupleGetItem in body of pattern
URL: https://github.com/apache/incubator-tvm/pull/5106#discussion_r398106710
 
 

 ##
 File path: src/relay/transforms/merge_composite.cc
 ##
 @@ -66,6 +66,31 @@ class MergeCompositeWrapper : public ExprMutator {
 return root;
   }
 
+  Expr ExtractPattern(const TupleGetItem& pattern, const Expr& root,
+  Map>* var_map, Map* call_map) {
+if (!root->IsInstance()) {
+  return Expr();
+}
+auto root_node = Downcast(root);
+if (pattern->index != root_node->index) {
+  return Expr();
+}
+if (pattern->tuple->IsInstance() &&
+root_node->tuple->IsInstance()) {
+  Expr new_arg;
+  if (call_map->find(pattern->tuple) != call_map->end()) {
+new_arg = (*call_map)[pattern->tuple];
+  } else {
+new_arg = ExtractPattern(Downcast(pattern->tuple),
+ Downcast(root_node->tuple),
+ var_map, call_map);
+call_map->Set(pattern->tuple, new_arg);
 
 Review comment:
   Call map is still only used for call nodes with this PR.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] icemelon9 commented on a change in pull request #5131: [Relay][OP] Register topi schedule for Relay fast_exp and fast_tanh

[GitBox]

icemelon9 commented on a change in pull request #5131: [Relay][OP] Register 
topi schedule for Relay fast_exp and fast_tanh
URL: https://github.com/apache/incubator-tvm/pull/5131#discussion_r398075515
 
 

 ##
 File path: tests/python/relay/test_op_fast_math.py
 ##
 @@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import numpy as np
+import tvm
+import tvm.relay as relay
+import topi
+from tvm import te
+from tvm.contrib import graph_runtime
+
+
+def test_fastmath():
+def test_apply(relay_op, name, f_numpy, low, high, step, dtype="float32"):
+a_np = np.arange(low, high, step).astype(dtype)
+b_np = f_numpy(a_np)
+
+x = relay.var("x", shape=a_np.shape, dtype="float32")
+y = relay_op(x)
+func = relay.Function([x], y)
+mod = tvm.IRModule.from_expr(func)
+
+with relay.build_config(opt_level=3, required_pass=['FastMath']):
+graph, lib, params = relay.build(mod, target="llvm", params=None)
+
+# check fast_math op is converted to lib function
+func_name = "fused_" + name
+assert lib.get_function(func_name)
+
+ctx = tvm.cpu(0)
+m = graph_runtime.create(graph, lib, ctx)
+# set inputs
+m.set_input('x', tvm.nd.array(a_np, ctx))
+m.set_input(**params)
+# execute
+m.run()
+# get outputs
+tvm_output = m.get_output(0)
+tvm.testing.assert_allclose(tvm_output.asnumpy(), b_np,
+rtol=1e-5, atol=1e-5)
+
+test_apply(relay.exp, "fast_exp", np.exp, low=-88, high=88, step=0.01)
+test_apply(relay.tanh, "fast_tanh", np.tanh, low=-10, high=10, step=0.01)
+
+if __name__ == "__main__":
 
 Review comment:
   fix the indent


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] icemelon9 commented on issue #5148: [Strategy][ARM CPU] Remove contrib spatial pack schedule of depthwise convolution

[GitBox]

icemelon9 commented on issue #5148: [Strategy][ARM CPU] Remove contrib spatial 
pack schedule of depthwise convolution
URL: https://github.com/apache/incubator-tvm/pull/5148#issuecomment-603999626
 
 
   Yes, let's comment out this implementation temporarily. Could you add a Todo 
with pointer to the issue in the discussion to both arm cpu strategy and the 
topi file?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] selo1412 commented on issue #5131: [Relay][OP] Register topi schedule for Relay fast_exp and fast_tanh

[GitBox]

selo1412 commented on issue #5131: [Relay][OP] Register topi schedule for Relay 
fast_exp and fast_tanh
URL: https://github.com/apache/incubator-tvm/pull/5131#issuecomment-603995475
 
 
   @comaniac @icemelon9 
   I took 
   tvm/tests/python/relay/test_pass_fast_math.py
   tvm/topi/tests/python/test_topi_math.py
   as reference to add unit test 
   tvm/tests/python/relay/test_op_fast_math.py .
   and I found some error in topi/tests/python/ make the commit 5 cannot be 
built.
   
   Is there any suggestions for the above situation?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] comaniac commented on a change in pull request #5134: [RELAY] Add MergeCompilerRegions pass

[GitBox]

comaniac commented on a change in pull request #5134: [RELAY] Add 
MergeCompilerRegions pass
URL: https://github.com/apache/incubator-tvm/pull/5134#discussion_r398051666
 
 

 ##
 File path: src/relay/transforms/merge_compiler_regions.cc
 ##
 @@ -0,0 +1,352 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * \file src/relay/transforms/merge_compiler_regions.cc
+ *
+ * \brief After operators have been annotated with the targets that support
+ * them, this pass creates regions of the operators for each target. It
+ * is guaranteed that the regions will have a topological ordering so that
+ * no data dependency issues exist.
+ *
+ * This pass only introduces annotations to indicate the regions.
+ * partition_graph must subsequently be called to lift these regions out
+ * as external functions.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "../analysis/annotated_region_set.h"
+
+
+namespace tvm {
+namespace relay {
+namespace partitioning {
+
+// Cache compiler_begin and compiler_end annotation ops for equivalence check 
to
+// reduce registry lookup overhead.
+static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+static const Op& compiler_end_op = Op::Get("annotation.compiler_end");
+
+/*! \brief This is a pre-requisite pass to merge-supported pass.
+ *  The AnnotateRestDefault pass will put "default" Compiler Annotations to
+ *  nodes that are not annotated already. This is there to ensure that the
+ *  user will not leave un-annotated nodes MergeCompilerRegions pass is run.
+ *  Why? Because, MergeCompilerRegions pass assumes every node to be annotated.
+ */
+class AnnotateRestDefault : public ExprMutator {
+ public:
+  explicit AnnotateRestDefault(const Expr& expr) {
+  regions_ = AnnotatedRegionSet::Create(expr, compiler_begin_op, 
compiler_end_op);
+  }
+
+  Expr Annotate(const Expr& expr) {
+// Its a function that is being passed on to annotate
+func_ = Downcast(expr);
+
+// Corner Case CC1 : If the last node does not belong
+// to a region nede to add a compiler_end
+auto region = regions_->GetRegion(func_->body);
+auto mutated_expr = this->VisitExpr(expr);
+if (!region.defined()) {
+  func_ = Downcast(mutated_expr);
+  // CC1 : add that compiler end after mutation
+  auto body = AddCompilerEnd_(func_->body);
+  func_ = Function(func_->params, body,
+   body->checked_type_, {}, DictAttrs());
+  return Downcast(func_);
+} else {
+  return mutated_expr;
+}
+  }
+
+  /*! \brief This function adds compiler ends to nodes that
+   * have a region AND they should not be arguments of the
+   * original function
+   * \param expr
+   * \return expr
+   */
+  Expr AddCompilerEnd(const Expr& expr) {
+auto region = regions_->GetRegion(expr);
+auto visited_expr = VisitExpr(expr);
+
+// The compiler ends are added to nodes that does have a region
+// AND they should not be arguments of the original function
+if (!region.defined() &&
+   std::find(func_->params.begin(),
+ func_->params.end(), visited_expr)
+   == func_->params.end()) {
+  return AddCompilerEnd_(visited_expr);
+} else {
+  return visited_expr;
+}
+  }
+
+  Expr AddCompilerEnd_(const Expr& expr) {
+const auto* end_op =
+  runtime::Registry::Get("relay.op.annotation._make.compiler_end");
+CHECK(end_op);
+Expr end = (*end_op)(expr, target_);
+return end;
+  }
+
+  Expr VisitExpr_(const CallNode* call) final {
+auto op_node = call->op.as();
+auto ret = GetRef(call);
+
+Array args;
+
+// Add compiler ends if the parent is supported
+for (auto arg : call->args) {
+  args.push_back(AddCompilerEnd(arg));
+}
+
+if (op_node == nullptr || call->attrs.as() == nullptr) {
+  // Skip annotatation ops, only add default compiler to actual compute 
nodes
+
+  auto region = regions_->GetRegion(ret);
+  if (!region.defined()) {
+// if the current node does not belong to annotated region
+// annotate the all incoming edges (args)
+// with 

[GitHub] [incubator-tvm] comaniac commented on a change in pull request #5134: [RELAY] Add MergeCompilerRegions pass

[GitBox]

comaniac commented on a change in pull request #5134: [RELAY] Add 
MergeCompilerRegions pass
URL: https://github.com/apache/incubator-tvm/pull/5134#discussion_r398037445
 
 

 ##
 File path: src/relay/transforms/merge_compiler_regions.cc
 ##
 @@ -0,0 +1,352 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * \file src/relay/transforms/merge_compiler_regions.cc
+ *
+ * \brief After operators have been annotated with the targets that support
+ * them, this pass creates regions of the operators for each target. It
+ * is guaranteed that the regions will have a topological ordering so that
+ * no data dependency issues exist.
+ *
+ * This pass only introduces annotations to indicate the regions.
+ * partition_graph must subsequently be called to lift these regions out
+ * as external functions.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "../analysis/annotated_region_set.h"
+
+
+namespace tvm {
+namespace relay {
+namespace partitioning {
+
+// Cache compiler_begin and compiler_end annotation ops for equivalence check 
to
+// reduce registry lookup overhead.
+static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+static const Op& compiler_end_op = Op::Get("annotation.compiler_end");
+
+/*! \brief This is a pre-requisite pass to merge-supported pass.
+ *  The AnnotateRestDefault pass will put "default" Compiler Annotations to
+ *  nodes that are not annotated already. This is there to ensure that the
+ *  user will not leave un-annotated nodes MergeCompilerRegions pass is run.
+ *  Why? Because, MergeCompilerRegions pass assumes every node to be annotated.
+ */
+class AnnotateRestDefault : public ExprMutator {
+ public:
+  explicit AnnotateRestDefault(const Expr& expr) {
+  regions_ = AnnotatedRegionSet::Create(expr, compiler_begin_op, 
compiler_end_op);
+  }
+
+  Expr Annotate(const Expr& expr) {
+// Its a function that is being passed on to annotate
+func_ = Downcast(expr);
+
+// Corner Case CC1 : If the last node does not belong
+// to a region nede to add a compiler_end
+auto region = regions_->GetRegion(func_->body);
+auto mutated_expr = this->VisitExpr(expr);
+if (!region.defined()) {
+  func_ = Downcast(mutated_expr);
+  // CC1 : add that compiler end after mutation
+  auto body = AddCompilerEnd_(func_->body);
+  func_ = Function(func_->params, body,
+   body->checked_type_, {}, DictAttrs());
+  return Downcast(func_);
+} else {
+  return mutated_expr;
+}
+  }
+
+  /*! \brief This function adds compiler ends to nodes that
+   * have a region AND they should not be arguments of the
+   * original function
+   * \param expr
+   * \return expr
+   */
+  Expr AddCompilerEnd(const Expr& expr) {
+auto region = regions_->GetRegion(expr);
+auto visited_expr = VisitExpr(expr);
+
+// The compiler ends are added to nodes that does have a region
+// AND they should not be arguments of the original function
+if (!region.defined() &&
+   std::find(func_->params.begin(),
+ func_->params.end(), visited_expr)
+   == func_->params.end()) {
+  return AddCompilerEnd_(visited_expr);
+} else {
+  return visited_expr;
+}
+  }
+
+  Expr AddCompilerEnd_(const Expr& expr) {
+const auto* end_op =
+  runtime::Registry::Get("relay.op.annotation._make.compiler_end");
+CHECK(end_op);
+Expr end = (*end_op)(expr, target_);
+return end;
+  }
+
+  Expr VisitExpr_(const CallNode* call) final {
+auto op_node = call->op.as();
+auto ret = GetRef(call);
+
+Array args;
+
+// Add compiler ends if the parent is supported
+for (auto arg : call->args) {
+  args.push_back(AddCompilerEnd(arg));
+}
+
+if (op_node == nullptr || call->attrs.as() == nullptr) {
+  // Skip annotatation ops, only add default compiler to actual compute 
nodes
+
+  auto region = regions_->GetRegion(ret);
+  if (!region.defined()) {
+// if the current node does not belong to annotated region
+// annotate the all incoming edges (args)
+// with 

[GitHub] [incubator-tvm] comaniac commented on a change in pull request #5134: [RELAY] Add MergeCompilerRegions pass

[GitBox]

comaniac commented on a change in pull request #5134: [RELAY] Add 
MergeCompilerRegions pass
URL: https://github.com/apache/incubator-tvm/pull/5134#discussion_r398044599
 
 

 ##
 File path: src/relay/transforms/merge_compiler_regions.cc
 ##
 @@ -0,0 +1,352 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * \file src/relay/transforms/merge_compiler_regions.cc
+ *
+ * \brief After operators have been annotated with the targets that support
+ * them, this pass creates regions of the operators for each target. It
+ * is guaranteed that the regions will have a topological ordering so that
+ * no data dependency issues exist.
+ *
+ * This pass only introduces annotations to indicate the regions.
+ * partition_graph must subsequently be called to lift these regions out
+ * as external functions.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "../analysis/annotated_region_set.h"
+
+
+namespace tvm {
+namespace relay {
+namespace partitioning {
+
+// Cache compiler_begin and compiler_end annotation ops for equivalence check 
to
+// reduce registry lookup overhead.
+static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+static const Op& compiler_end_op = Op::Get("annotation.compiler_end");
+
+/*! \brief This is a pre-requisite pass to merge-supported pass.
+ *  The AnnotateRestDefault pass will put "default" Compiler Annotations to
+ *  nodes that are not annotated already. This is there to ensure that the
+ *  user will not leave un-annotated nodes MergeCompilerRegions pass is run.
+ *  Why? Because, MergeCompilerRegions pass assumes every node to be annotated.
+ */
+class AnnotateRestDefault : public ExprMutator {
+ public:
+  explicit AnnotateRestDefault(const Expr& expr) {
+  regions_ = AnnotatedRegionSet::Create(expr, compiler_begin_op, 
compiler_end_op);
+  }
+
+  Expr Annotate(const Expr& expr) {
+// Its a function that is being passed on to annotate
+func_ = Downcast(expr);
+
+// Corner Case CC1 : If the last node does not belong
+// to a region nede to add a compiler_end
+auto region = regions_->GetRegion(func_->body);
+auto mutated_expr = this->VisitExpr(expr);
+if (!region.defined()) {
+  func_ = Downcast(mutated_expr);
+  // CC1 : add that compiler end after mutation
+  auto body = AddCompilerEnd_(func_->body);
+  func_ = Function(func_->params, body,
+   body->checked_type_, {}, DictAttrs());
+  return Downcast(func_);
+} else {
+  return mutated_expr;
+}
+  }
+
+  /*! \brief This function adds compiler ends to nodes that
+   * have a region AND they should not be arguments of the
+   * original function
+   * \param expr
+   * \return expr
+   */
+  Expr AddCompilerEnd(const Expr& expr) {
+auto region = regions_->GetRegion(expr);
+auto visited_expr = VisitExpr(expr);
+
+// The compiler ends are added to nodes that does have a region
+// AND they should not be arguments of the original function
+if (!region.defined() &&
+   std::find(func_->params.begin(),
+ func_->params.end(), visited_expr)
+   == func_->params.end()) {
+  return AddCompilerEnd_(visited_expr);
+} else {
+  return visited_expr;
+}
+  }
+
+  Expr AddCompilerEnd_(const Expr& expr) {
+const auto* end_op =
+  runtime::Registry::Get("relay.op.annotation._make.compiler_end");
+CHECK(end_op);
+Expr end = (*end_op)(expr, target_);
+return end;
+  }
+
+  Expr VisitExpr_(const CallNode* call) final {
+auto op_node = call->op.as();
+auto ret = GetRef(call);
+
+Array args;
+
+// Add compiler ends if the parent is supported
+for (auto arg : call->args) {
+  args.push_back(AddCompilerEnd(arg));
+}
+
+if (op_node == nullptr || call->attrs.as() == nullptr) {
+  // Skip annotatation ops, only add default compiler to actual compute 
nodes
+
+  auto region = regions_->GetRegion(ret);
+  if (!region.defined()) {
+// if the current node does not belong to annotated region
+// annotate the all incoming edges (args)
+// with 

[GitHub] [incubator-tvm] comaniac commented on a change in pull request #5134: [RELAY] Add MergeCompilerRegions pass

[GitBox]

comaniac commented on a change in pull request #5134: [RELAY] Add 
MergeCompilerRegions pass
URL: https://github.com/apache/incubator-tvm/pull/5134#discussion_r398041335
 
 

 ##
 File path: src/relay/transforms/merge_compiler_regions.cc
 ##
 @@ -0,0 +1,352 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * \file src/relay/transforms/merge_compiler_regions.cc
+ *
+ * \brief After operators have been annotated with the targets that support
+ * them, this pass creates regions of the operators for each target. It
+ * is guaranteed that the regions will have a topological ordering so that
+ * no data dependency issues exist.
+ *
+ * This pass only introduces annotations to indicate the regions.
+ * partition_graph must subsequently be called to lift these regions out
+ * as external functions.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "../analysis/annotated_region_set.h"
+
+
+namespace tvm {
+namespace relay {
+namespace partitioning {
+
+// Cache compiler_begin and compiler_end annotation ops for equivalence check 
to
+// reduce registry lookup overhead.
+static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+static const Op& compiler_end_op = Op::Get("annotation.compiler_end");
+
+/*! \brief This is a pre-requisite pass to merge-supported pass.
+ *  The AnnotateRestDefault pass will put "default" Compiler Annotations to
+ *  nodes that are not annotated already. This is there to ensure that the
+ *  user will not leave un-annotated nodes MergeCompilerRegions pass is run.
+ *  Why? Because, MergeCompilerRegions pass assumes every node to be annotated.
+ */
+class AnnotateRestDefault : public ExprMutator {
+ public:
+  explicit AnnotateRestDefault(const Expr& expr) {
+  regions_ = AnnotatedRegionSet::Create(expr, compiler_begin_op, 
compiler_end_op);
+  }
+
+  Expr Annotate(const Expr& expr) {
+// Its a function that is being passed on to annotate
+func_ = Downcast(expr);
+
+// Corner Case CC1 : If the last node does not belong
+// to a region nede to add a compiler_end
+auto region = regions_->GetRegion(func_->body);
+auto mutated_expr = this->VisitExpr(expr);
+if (!region.defined()) {
+  func_ = Downcast(mutated_expr);
+  // CC1 : add that compiler end after mutation
+  auto body = AddCompilerEnd_(func_->body);
+  func_ = Function(func_->params, body,
+   body->checked_type_, {}, DictAttrs());
+  return Downcast(func_);
+} else {
+  return mutated_expr;
+}
+  }
+
+  /*! \brief This function adds compiler ends to nodes that
+   * have a region AND they should not be arguments of the
+   * original function
+   * \param expr
+   * \return expr
+   */
+  Expr AddCompilerEnd(const Expr& expr) {
+auto region = regions_->GetRegion(expr);
+auto visited_expr = VisitExpr(expr);
+
+// The compiler ends are added to nodes that does have a region
+// AND they should not be arguments of the original function
+if (!region.defined() &&
+   std::find(func_->params.begin(),
+ func_->params.end(), visited_expr)
+   == func_->params.end()) {
+  return AddCompilerEnd_(visited_expr);
+} else {
+  return visited_expr;
+}
+  }
+
+  Expr AddCompilerEnd_(const Expr& expr) {
+const auto* end_op =
+  runtime::Registry::Get("relay.op.annotation._make.compiler_end");
+CHECK(end_op);
+Expr end = (*end_op)(expr, target_);
+return end;
+  }
+
+  Expr VisitExpr_(const CallNode* call) final {
+auto op_node = call->op.as();
+auto ret = GetRef(call);
+
+Array args;
+
+// Add compiler ends if the parent is supported
+for (auto arg : call->args) {
+  args.push_back(AddCompilerEnd(arg));
+}
+
+if (op_node == nullptr || call->attrs.as() == nullptr) {
+  // Skip annotatation ops, only add default compiler to actual compute 
nodes
+
+  auto region = regions_->GetRegion(ret);
+  if (!region.defined()) {
+// if the current node does not belong to annotated region
+// annotate the all incoming edges (args)
+// with 

[GitHub] [incubator-tvm] yzhliu commented on a change in pull request #5092: [TIR][PASS] dtype rewrite for indexing variables

[GitBox]

yzhliu commented on a change in pull request #5092: [TIR][PASS] dtype rewrite 
for indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r398041071
 
 

 ##
 File path: src/tir/pass/narrow_datatype.cc
 ##
 @@ -0,0 +1,373 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file narrow_datatype.cc
+ * \brief narrow the datatype of indexing vars
+ */
+
+#include 
+#include 
+#include "../../arith/ir_mutator_with_analyzer.h"
+#include "../../arith/ir_visitor_with_analyzer.h"
+
+namespace tvm {
+namespace tir {
+
+// This pass narrows indexing expressions (like StoreNode::Index)
+// that trivially fit into i32 to i32. Considering that i32 indices
+// may be more efficient on some backends (while i64 may be more
+// efficient on others, like llvm), we may want this pass when i32
+// indices are more efficient.
+//
+// For Var v, we determine its dtype by examining all the PrimExpr
+// that contains v, denoted by E = {e_0 = v, e_1, e_2, ..., e_k}.
+// If all expressions in E fit into i32, then we think v can be narrowed
+// to i32.
+//
+// To make an indexing expression i32, we must make sure that every
+// component of that expression is of dtype i32. So besides Var, we
+// rewrite the following inside an indexing expression
+// - Var
+// - IntImm
+// - Cast
+//
+// Algorithm:
+// - Use DataTypeVisitor to determine whether a Var can be narrowed or not.
+// - Use DataTypeRewritter to rewrite the components of an indexing expression.
+
+using arith::Analyzer;
+using arith::IRMutatorWithAnalyzer;
+using arith::ConstIntBound;
+
+class DataTypeVisitor final : public StmtExprVisitor {
+ public:
+  explicit DataTypeVisitor(int target_bits)
+: bits_(target_bits), target_bits_(target_bits) {}
+
+  void VisitExpr(const PrimExpr& e) {
+if (e.dtype().is_int()) {
+  int bits = max_bits_;
+  ConstIntBound bound = analyzer_.const_int_bound(e);
+  int64_t ubound = Downcast(max_value(DataType::Int(target_bits_)))->value;
+  int64_t lbound = Downcast(min_value(DataType::Int(target_bits_)))->value;
+  if (e.dtype().bits() <= target_bits_ ||
 
 Review comment:
   Do you have an example? We previously reviewed some of the scenarios, not 
seeing needs doing so. 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] comaniac commented on issue #5148: [Strategy][ARM CPU] Remove contrib spatial pack schedule of depthwise convolution

[GitBox]

comaniac commented on issue #5148: [Strategy][ARM CPU] Remove contrib spatial 
pack schedule of depthwise convolution
URL: https://github.com/apache/incubator-tvm/pull/5148#issuecomment-603961524
 
 
   > After thinking it a bit more, lower the plevel could only change the 
default schedule. However, when the users tune it by themselves, they will 
still introduce this `contrib` schedule into their tuning task. Before we could 
solve this problem, I would like to comment it out (but keep the code there). 
What about your advices? @comaniac @icemelon9
   
   You are right. That's the intention of the `plevel` in strategy. If this 
schedule has a potential correctness issue, we could have the following 3 
solutions:
   
   1. Suggest users enabling correctness checking when tuning (i.e., let 
AutoTVM check correctness for each schedule config), but this will also check 
the other task that does not have this issue, making the tuning time even 
longer.
   
   2. Add an optional when registering an AutoTVM task to enforce correctness 
checking, but this requires a change to not only the registration but AutoTVM 
mechanism.
   
   3. Comment out the schedule and let it back after the issue has been 
resolved. The problem is that we may never let it back if no one is dedicated 
to fix it...
   
   @icemelon9 do you have any suggestions?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] FrozenGene commented on issue #5148: [Strategy][ARM CPU] Remove contrib spatial pack schedule of depthwise convolution

[GitBox]

FrozenGene commented on issue #5148: [Strategy][ARM CPU] Remove contrib spatial 
pack schedule of depthwise convolution
URL: https://github.com/apache/incubator-tvm/pull/5148#issuecomment-603956217
 
 
   After thinking it a bit more, lower the plevel could only change the default 
schedule. However, when the users tune it by themselves, they will still 
introduce this `contrib` schedule into their tuning task. Before we could solve 
this problem, I would like to comment it out (but keep the code there). What 
about your advices? @comaniac @icemelon9  


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] ANSHUMAN87 opened a new pull request #5149: [WIP][Relay][Frontend] Chainer frontend introduced

[GitBox]

ANSHUMAN87 opened a new pull request #5149: [WIP][Relay][Frontend] Chainer 
frontend introduced
URL: https://github.com/apache/incubator-tvm/pull/5149
 
 
   Chainer frontend is added among other Relay frontends.
   
   Note: This is just a skeleton now. And it is under active development.
   Please do not refer for review.
   Once it is ready for review, will update and include respective reviewers.
   
   However, if anyone has any specific requirement from chainer framework, 
please feel free to post here. Will sure take it up.
   
   
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on issue #5030: [RELAY] Added a AnnotatedRegion utility class

[GitBox]

zhiics commented on issue #5030: [RELAY] Added a AnnotatedRegion utility class
URL: https://github.com/apache/incubator-tvm/pull/5030#issuecomment-603949577
 
 
   @jwfromm could you PTAL?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5134: [RELAY] Add MergeCompilerRegions pass

[GitBox]

zhiics commented on a change in pull request #5134: [RELAY] Add 
MergeCompilerRegions pass
URL: https://github.com/apache/incubator-tvm/pull/5134#discussion_r397987916
 
 

 ##
 File path: src/relay/transforms/annotate_target.cc
 ##
 @@ -38,46 +38,144 @@ class AnnotateTargetWrapper : public ExprMutator {
  public:
   explicit AnnotateTargetWrapper(const std::string& target) : target_(target) 
{}
 
+  Expr Annotate(const Expr& expr) {
+return InsertEnd(Mutate(expr));
+  }
+
+  bool IsSupported(const Expr& expr) {
+if (expr->IsInstance()) {
+  Call call = Downcast(expr);
+  auto fannotate = Op::GetAttr("target." + target_);
+  Op op = Downcast(call->op);
+  CHECK(op.defined());
+  if (fannotate.count(op)) {
+return fannotate[op](call->attrs, call->args);
+  }
+}
+return false;
+  }
+
+  Expr InsertEnd(const Expr& arg) {
+if (IsSupported(arg)) {
+  const auto *end_op =
+runtime::Registry::Get("relay.op.annotation._make.compiler_end");
+  CHECK(end_op);
+  Expr end = (*end_op)(arg, target_);
+  return end;
+}
+return arg;
+  }
+
   Expr VisitExpr_(const CallNode* cn) {
 // TODO(@zhiics, @comaniac) Handle composite functions.
 auto new_e = ExprMutator::VisitExpr_(cn);
 
 Call call = Downcast(new_e);
-static auto fannotate = Op::GetAttr("target." + 
target_);
-Op op = Downcast(call->op);
-CHECK(op.defined());
-
-if (fannotate.count(op)) {
-  bool external = fannotate[op](call->attrs, call->args);
-  if (external) {
-tvm::Array compiler_begins;
-for (const auto& it : call->args) {
-  const auto* begin_op =
-runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
-  CHECK(begin_op);
-  Expr begin = (*begin_op)(it, target_);
-  compiler_begins.push_back(begin);
-}
-Expr update_call = Call(call->op, compiler_begins, call->attrs);
-const auto* end_op =
-  runtime::Registry::Get("relay.op.annotation._make.compiler_end");
-CHECK(end_op);
-Expr end = (*end_op)(update_call, target_);
-return end;
+
+// add end annotations if the args are supported
+Array compiler_ends;
+for (const auto& it : call->args) {
+  compiler_ends.push_back(InsertEnd(it));
+}
+call = Call(call->op, compiler_ends, call->attrs);
+
+// add begin annotations if the call node is supported
+if (IsSupported(call)) {
+  tvm::Array compiler_begins;
+  for (const auto& it : call->args) {
+const auto* begin_op =
+  runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
+CHECK(begin_op);
+Expr begin = (*begin_op)(it, target_);
+compiler_begins.push_back(begin);
   }
-} else {
-  LOG(WARNING) << op->name << " in " << target_
-   << " is not registered. It will be executed on CPU.";
+  call = Call(call->op, compiler_begins, call->attrs);
 }
-return new_e;
+
+return std::move(call);
+  }
+
+  Expr VisitExpr_(const TupleNode *op) {
 
 Review comment:
   use Google C++ code style: `TupleNode* op`, same to the other changes or 
this PR.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5134: [RELAY] Add MergeCompilerRegions pass

[GitBox]

zhiics commented on a change in pull request #5134: [RELAY] Add 
MergeCompilerRegions pass
URL: https://github.com/apache/incubator-tvm/pull/5134#discussion_r397998462
 
 

 ##
 File path: src/relay/transforms/merge_compiler_regions.cc
 ##
 @@ -0,0 +1,352 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * \file src/relay/transforms/merge_compiler_regions.cc
+ *
+ * \brief After operators have been annotated with the targets that support
+ * them, this pass creates regions of the operators for each target. It
+ * is guaranteed that the regions will have a topological ordering so that
+ * no data dependency issues exist.
+ *
+ * This pass only introduces annotations to indicate the regions.
+ * partition_graph must subsequently be called to lift these regions out
+ * as external functions.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "../analysis/annotated_region_set.h"
+
+
+namespace tvm {
+namespace relay {
+namespace partitioning {
+
+// Cache compiler_begin and compiler_end annotation ops for equivalence check 
to
+// reduce registry lookup overhead.
+static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+static const Op& compiler_end_op = Op::Get("annotation.compiler_end");
+
+/*! \brief This is a pre-requisite pass to merge-supported pass.
+ *  The AnnotateRestDefault pass will put "default" Compiler Annotations to
+ *  nodes that are not annotated already. This is there to ensure that the
+ *  user will not leave un-annotated nodes MergeCompilerRegions pass is run.
+ *  Why? Because, MergeCompilerRegions pass assumes every node to be annotated.
+ */
+class AnnotateRestDefault : public ExprMutator {
+ public:
+  explicit AnnotateRestDefault(const Expr& expr) {
+  regions_ = AnnotatedRegionSet::Create(expr, compiler_begin_op, 
compiler_end_op);
+  }
+
+  Expr Annotate(const Expr& expr) {
+// Its a function that is being passed on to annotate
+func_ = Downcast(expr);
+
+// Corner Case CC1 : If the last node does not belong
+// to a region nede to add a compiler_end
+auto region = regions_->GetRegion(func_->body);
+auto mutated_expr = this->VisitExpr(expr);
+if (!region.defined()) {
+  func_ = Downcast(mutated_expr);
+  // CC1 : add that compiler end after mutation
+  auto body = AddCompilerEnd_(func_->body);
+  func_ = Function(func_->params, body,
+   body->checked_type_, {}, DictAttrs());
+  return Downcast(func_);
+} else {
+  return mutated_expr;
+}
+  }
+
+  /*! \brief This function adds compiler ends to nodes that
+   * have a region AND they should not be arguments of the
+   * original function
+   * \param expr
+   * \return expr
+   */
+  Expr AddCompilerEnd(const Expr& expr) {
+auto region = regions_->GetRegion(expr);
+auto visited_expr = VisitExpr(expr);
+
+// The compiler ends are added to nodes that does have a region
+// AND they should not be arguments of the original function
+if (!region.defined() &&
+   std::find(func_->params.begin(),
+ func_->params.end(), visited_expr)
+   == func_->params.end()) {
+  return AddCompilerEnd_(visited_expr);
+} else {
+  return visited_expr;
+}
+  }
+
+  Expr AddCompilerEnd_(const Expr& expr) {
+const auto* end_op =
+  runtime::Registry::Get("relay.op.annotation._make.compiler_end");
+CHECK(end_op);
+Expr end = (*end_op)(expr, target_);
+return end;
+  }
+
+  Expr VisitExpr_(const CallNode* call) final {
+auto op_node = call->op.as();
+auto ret = GetRef(call);
+
+Array args;
+
+// Add compiler ends if the parent is supported
+for (auto arg : call->args) {
+  args.push_back(AddCompilerEnd(arg));
+}
+
+if (op_node == nullptr || call->attrs.as() == nullptr) {
+  // Skip annotatation ops, only add default compiler to actual compute 
nodes
+
+  auto region = regions_->GetRegion(ret);
+  if (!region.defined()) {
+// if the current node does not belong to annotated region
+// annotate the all incoming edges (args)
+// with 

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5134: [RELAY] Add MergeCompilerRegions pass

[GitBox]

zhiics commented on a change in pull request #5134: [RELAY] Add 
MergeCompilerRegions pass
URL: https://github.com/apache/incubator-tvm/pull/5134#discussion_r397993884
 
 

 ##
 File path: src/relay/transforms/merge_compiler_regions.cc
 ##
 @@ -0,0 +1,352 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * \file src/relay/transforms/merge_compiler_regions.cc
+ *
+ * \brief After operators have been annotated with the targets that support
+ * them, this pass creates regions of the operators for each target. It
+ * is guaranteed that the regions will have a topological ordering so that
+ * no data dependency issues exist.
+ *
+ * This pass only introduces annotations to indicate the regions.
+ * partition_graph must subsequently be called to lift these regions out
+ * as external functions.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "../analysis/annotated_region_set.h"
+
+
+namespace tvm {
+namespace relay {
+namespace partitioning {
+
+// Cache compiler_begin and compiler_end annotation ops for equivalence check 
to
+// reduce registry lookup overhead.
+static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+static const Op& compiler_end_op = Op::Get("annotation.compiler_end");
+
+/*! \brief This is a pre-requisite pass to merge-supported pass.
+ *  The AnnotateRestDefault pass will put "default" Compiler Annotations to
+ *  nodes that are not annotated already. This is there to ensure that the
+ *  user will not leave un-annotated nodes MergeCompilerRegions pass is run.
+ *  Why? Because, MergeCompilerRegions pass assumes every node to be annotated.
+ */
+class AnnotateRestDefault : public ExprMutator {
+ public:
+  explicit AnnotateRestDefault(const Expr& expr) {
+  regions_ = AnnotatedRegionSet::Create(expr, compiler_begin_op, 
compiler_end_op);
+  }
+
+  Expr Annotate(const Expr& expr) {
+// Its a function that is being passed on to annotate
+func_ = Downcast(expr);
+
+// Corner Case CC1 : If the last node does not belong
+// to a region nede to add a compiler_end
+auto region = regions_->GetRegion(func_->body);
+auto mutated_expr = this->VisitExpr(expr);
+if (!region.defined()) {
+  func_ = Downcast(mutated_expr);
+  // CC1 : add that compiler end after mutation
+  auto body = AddCompilerEnd_(func_->body);
+  func_ = Function(func_->params, body,
+   body->checked_type_, {}, DictAttrs());
+  return Downcast(func_);
+} else {
+  return mutated_expr;
+}
+  }
+
+  /*! \brief This function adds compiler ends to nodes that
+   * have a region AND they should not be arguments of the
+   * original function
+   * \param expr
+   * \return expr
+   */
+  Expr AddCompilerEnd(const Expr& expr) {
+auto region = regions_->GetRegion(expr);
+auto visited_expr = VisitExpr(expr);
+
+// The compiler ends are added to nodes that does have a region
+// AND they should not be arguments of the original function
+if (!region.defined() &&
+   std::find(func_->params.begin(),
+ func_->params.end(), visited_expr)
+   == func_->params.end()) {
+  return AddCompilerEnd_(visited_expr);
+} else {
+  return visited_expr;
+}
+  }
+
+  Expr AddCompilerEnd_(const Expr& expr) {
+const auto* end_op =
+  runtime::Registry::Get("relay.op.annotation._make.compiler_end");
+CHECK(end_op);
+Expr end = (*end_op)(expr, target_);
+return end;
+  }
+
+  Expr VisitExpr_(const CallNode* call) final {
+auto op_node = call->op.as();
+auto ret = GetRef(call);
+
+Array args;
+
+// Add compiler ends if the parent is supported
+for (auto arg : call->args) {
+  args.push_back(AddCompilerEnd(arg));
+}
+
+if (op_node == nullptr || call->attrs.as() == nullptr) {
+  // Skip annotatation ops, only add default compiler to actual compute 
nodes
+
+  auto region = regions_->GetRegion(ret);
+  if (!region.defined()) {
+// if the current node does not belong to annotated region
+// annotate the all incoming edges (args)
+// with 

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5134: [RELAY] Add MergeCompilerRegions pass

[GitBox]

zhiics commented on a change in pull request #5134: [RELAY] Add 
MergeCompilerRegions pass
URL: https://github.com/apache/incubator-tvm/pull/5134#discussion_r397994225
 
 

 ##
 File path: src/relay/transforms/merge_compiler_regions.cc
 ##
 @@ -0,0 +1,352 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * \file src/relay/transforms/merge_compiler_regions.cc
+ *
+ * \brief After operators have been annotated with the targets that support
+ * them, this pass creates regions of the operators for each target. It
+ * is guaranteed that the regions will have a topological ordering so that
+ * no data dependency issues exist.
+ *
+ * This pass only introduces annotations to indicate the regions.
+ * partition_graph must subsequently be called to lift these regions out
+ * as external functions.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "../analysis/annotated_region_set.h"
+
+
+namespace tvm {
+namespace relay {
+namespace partitioning {
+
+// Cache compiler_begin and compiler_end annotation ops for equivalence check 
to
+// reduce registry lookup overhead.
+static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+static const Op& compiler_end_op = Op::Get("annotation.compiler_end");
+
+/*! \brief This is a pre-requisite pass to merge-supported pass.
+ *  The AnnotateRestDefault pass will put "default" Compiler Annotations to
+ *  nodes that are not annotated already. This is there to ensure that the
+ *  user will not leave un-annotated nodes MergeCompilerRegions pass is run.
+ *  Why? Because, MergeCompilerRegions pass assumes every node to be annotated.
+ */
+class AnnotateRestDefault : public ExprMutator {
+ public:
+  explicit AnnotateRestDefault(const Expr& expr) {
+  regions_ = AnnotatedRegionSet::Create(expr, compiler_begin_op, 
compiler_end_op);
+  }
+
+  Expr Annotate(const Expr& expr) {
+// Its a function that is being passed on to annotate
+func_ = Downcast(expr);
+
+// Corner Case CC1 : If the last node does not belong
+// to a region nede to add a compiler_end
+auto region = regions_->GetRegion(func_->body);
+auto mutated_expr = this->VisitExpr(expr);
+if (!region.defined()) {
+  func_ = Downcast(mutated_expr);
+  // CC1 : add that compiler end after mutation
+  auto body = AddCompilerEnd_(func_->body);
+  func_ = Function(func_->params, body,
+   body->checked_type_, {}, DictAttrs());
+  return Downcast(func_);
+} else {
+  return mutated_expr;
+}
+  }
+
+  /*! \brief This function adds compiler ends to nodes that
+   * have a region AND they should not be arguments of the
+   * original function
+   * \param expr
+   * \return expr
+   */
+  Expr AddCompilerEnd(const Expr& expr) {
+auto region = regions_->GetRegion(expr);
+auto visited_expr = VisitExpr(expr);
+
+// The compiler ends are added to nodes that does have a region
+// AND they should not be arguments of the original function
+if (!region.defined() &&
+   std::find(func_->params.begin(),
+ func_->params.end(), visited_expr)
+   == func_->params.end()) {
+  return AddCompilerEnd_(visited_expr);
+} else {
+  return visited_expr;
+}
+  }
+
+  Expr AddCompilerEnd_(const Expr& expr) {
+const auto* end_op =
+  runtime::Registry::Get("relay.op.annotation._make.compiler_end");
+CHECK(end_op);
+Expr end = (*end_op)(expr, target_);
+return end;
+  }
+
+  Expr VisitExpr_(const CallNode* call) final {
+auto op_node = call->op.as();
+auto ret = GetRef(call);
+
+Array args;
+
+// Add compiler ends if the parent is supported
+for (auto arg : call->args) {
+  args.push_back(AddCompilerEnd(arg));
+}
+
+if (op_node == nullptr || call->attrs.as() == nullptr) {
+  // Skip annotatation ops, only add default compiler to actual compute 
nodes
+
+  auto region = regions_->GetRegion(ret);
+  if (!region.defined()) {
+// if the current node does not belong to annotated region
+// annotate the all incoming edges (args)
+// with 

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5134: [RELAY] Add MergeCompilerRegions pass

[GitBox]

zhiics commented on a change in pull request #5134: [RELAY] Add 
MergeCompilerRegions pass
URL: https://github.com/apache/incubator-tvm/pull/5134#discussion_r397986878
 
 

 ##
 File path: src/relay/transforms/annotate_target.cc
 ##
 @@ -38,46 +38,144 @@ class AnnotateTargetWrapper : public ExprMutator {
  public:
   explicit AnnotateTargetWrapper(const std::string& target) : target_(target) 
{}
 
+  Expr Annotate(const Expr& expr) {
+return InsertEnd(Mutate(expr));
+  }
+
+  bool IsSupported(const Expr& expr) {
+if (expr->IsInstance()) {
+  Call call = Downcast(expr);
+  auto fannotate = Op::GetAttr("target." + target_);
+  Op op = Downcast(call->op);
+  CHECK(op.defined());
+  if (fannotate.count(op)) {
+return fannotate[op](call->attrs, call->args);
+  }
+}
+return false;
+  }
+
+  Expr InsertEnd(const Expr& arg) {
+if (IsSupported(arg)) {
+  const auto *end_op =
+runtime::Registry::Get("relay.op.annotation._make.compiler_end");
+  CHECK(end_op);
+  Expr end = (*end_op)(arg, target_);
+  return end;
+}
+return arg;
+  }
+
   Expr VisitExpr_(const CallNode* cn) {
 // TODO(@zhiics, @comaniac) Handle composite functions.
 auto new_e = ExprMutator::VisitExpr_(cn);
 
 Call call = Downcast(new_e);
-static auto fannotate = Op::GetAttr("target." + 
target_);
-Op op = Downcast(call->op);
-CHECK(op.defined());
-
-if (fannotate.count(op)) {
-  bool external = fannotate[op](call->attrs, call->args);
-  if (external) {
-tvm::Array compiler_begins;
-for (const auto& it : call->args) {
-  const auto* begin_op =
-runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
-  CHECK(begin_op);
-  Expr begin = (*begin_op)(it, target_);
-  compiler_begins.push_back(begin);
-}
-Expr update_call = Call(call->op, compiler_begins, call->attrs);
-const auto* end_op =
-  runtime::Registry::Get("relay.op.annotation._make.compiler_end");
-CHECK(end_op);
-Expr end = (*end_op)(update_call, target_);
-return end;
+
+// add end annotations if the args are supported
+Array compiler_ends;
+for (const auto& it : call->args) {
+  compiler_ends.push_back(InsertEnd(it));
+}
+call = Call(call->op, compiler_ends, call->attrs);
+
+// add begin annotations if the call node is supported
+if (IsSupported(call)) {
+  tvm::Array compiler_begins;
+  for (const auto& it : call->args) {
+const auto* begin_op =
 
 Review comment:
   move this outside of the for loop


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5134: [RELAY] Add MergeCompilerRegions pass

[GitBox]

zhiics commented on a change in pull request #5134: [RELAY] Add 
MergeCompilerRegions pass
URL: https://github.com/apache/incubator-tvm/pull/5134#discussion_r397991560
 
 

 ##
 File path: src/relay/transforms/merge_compiler_regions.cc
 ##
 @@ -0,0 +1,352 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * \file src/relay/transforms/merge_compiler_regions.cc
+ *
+ * \brief After operators have been annotated with the targets that support
+ * them, this pass creates regions of the operators for each target. It
+ * is guaranteed that the regions will have a topological ordering so that
+ * no data dependency issues exist.
+ *
+ * This pass only introduces annotations to indicate the regions.
+ * partition_graph must subsequently be called to lift these regions out
+ * as external functions.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "../analysis/annotated_region_set.h"
+
+
+namespace tvm {
+namespace relay {
+namespace partitioning {
+
+// Cache compiler_begin and compiler_end annotation ops for equivalence check 
to
+// reduce registry lookup overhead.
+static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+static const Op& compiler_end_op = Op::Get("annotation.compiler_end");
+
+/*! \brief This is a pre-requisite pass to merge-supported pass.
+ *  The AnnotateRestDefault pass will put "default" Compiler Annotations to
+ *  nodes that are not annotated already. This is there to ensure that the
+ *  user will not leave un-annotated nodes MergeCompilerRegions pass is run.
+ *  Why? Because, MergeCompilerRegions pass assumes every node to be annotated.
+ */
+class AnnotateRestDefault : public ExprMutator {
+ public:
+  explicit AnnotateRestDefault(const Expr& expr) {
+  regions_ = AnnotatedRegionSet::Create(expr, compiler_begin_op, 
compiler_end_op);
+  }
+
+  Expr Annotate(const Expr& expr) {
+// Its a function that is being passed on to annotate
+func_ = Downcast(expr);
+
+// Corner Case CC1 : If the last node does not belong
+// to a region nede to add a compiler_end
+auto region = regions_->GetRegion(func_->body);
+auto mutated_expr = this->VisitExpr(expr);
+if (!region.defined()) {
+  func_ = Downcast(mutated_expr);
+  // CC1 : add that compiler end after mutation
+  auto body = AddCompilerEnd_(func_->body);
+  func_ = Function(func_->params, body,
+   body->checked_type_, {}, DictAttrs());
+  return Downcast(func_);
+} else {
+  return mutated_expr;
+}
+  }
+
+  /*! \brief This function adds compiler ends to nodes that
+   * have a region AND they should not be arguments of the
+   * original function
+   * \param expr
 
 Review comment:
   Complete it, same for return


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] FrozenGene opened a new pull request #5148: [Strategy][ARM CPU] Low the plevel of contrib spatial pack of depthwise conv

[GitBox]

FrozenGene opened a new pull request #5148: [Strategy][ARM CPU] Low the plevel 
of contrib spatial pack of depthwise conv
URL: https://github.com/apache/incubator-tvm/pull/5148
 
 
   The spatial pack schedule of depthwise convolution is under the tag of 
`contrib` before introducing op strategy. However, as this discuss: 
https://discuss.tvm.ai/t/autotuner-incorrect-result-after-tuning-mobilenetv2-on-arm-cpu/6088,
 we maybe will meet incorrect result use this schedule. I ever met this problem 
on some hardware platform too. However, it is not 100% could produce and 
happened rarely. Correct result should be firstly, so let us make our previous 
depthwise schedule be the default, not the `contrib` spatial pack schedule.
   
   @icemelon9 Could you help to review the code? 
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5143: [RELAY] Re-wrote the Graph Partitioner to support multiple outputs

[GitBox]

zhiics commented on a change in pull request #5143: [RELAY] Re-wrote the Graph 
Partitioner to support multiple outputs
URL: https://github.com/apache/incubator-tvm/pull/5143#discussion_r397979319
 
 

 ##
 File path: src/relay/transforms/partition_graph.cc
 ##
 @@ -165,101 +162,142 @@ class Partitioner : public ExprMutator {
   // Traverse the rest graph.
   auto input_expr = VisitExpr(call->args[0]);
 
-  // Replace the begin annotation with an external call input variable.
-  auto compiler_attrs = call->attrs.as();
+  AnnotatedRegion sg = GetRegion(GetRef(call));
+  int index = GetArgIdx(sg, GetRef(call));
+  CHECK_NE(index, -1);
   // The type of the created variable is the same as the compiler_begin
   // node.
-  auto var = Var(compiler_attrs->compiler + "_input" + 
std::to_string(var_id_++),
-   call->checked_type_);
-
-  // Find the corresponding subgraph and add the argument.
-  auto subgraph = GetSubgraph(GetRef(call));
-  if (!subgraph) {
-throw Error(ErrorBuilder()
-<< "Cannot find the corresponding subgraph for start 
annotation:\n"
-<< AsText(GetRef(call), false));
-  }
-  subgraph->args.push_back({var, input_expr});
+  std::string target = call->attrs.as()->compiler;
+  std::string varname = target + "_" + std::to_string(sg->GetID())
++ "_i" + std::to_string(index);
+  auto var = Var(varname, GetRef(call)->checked_type_);
+
+  auto cand = std::make_pair(var, input_expr);
+  if (std::find(region_args[sg].begin(),
+region_args[sg].end(), cand) == region_args[sg].end()) {
+region_args[sg].push_back(cand);
+ }
+
   return std::move(var);
 } else {
   CHECK_EQ(call->op, compiler_end_op);
   // The annotation node is inserted on edge so it must have only one 
argument.
   CHECK_EQ(call->args.size(), 1U);
 
-  auto compiler_attrs = call->attrs.as();
+  AnnotatedRegion region = GetRegion(GetRef(call));
 
-  // Check if the argument already belongs to an existing subgraph
-  auto subgraph = GetSubgraph(call->args[0]);
-  if (!subgraph) {
-auto ret = this->subgraphs_.emplace(std::make_shared());
-subgraph = *ret.first;
-subgraph->nodes.insert(call->args[0]);
-subgraph->id = this->subgraph_id_++;
-  }
-  subgraph->nodes.insert(GetRef(call));
+  // TODO(@manupa-arm) : need to use the parent function (to which region
+  // belongs to) name/key for the funtions that are created
+  BaseFunc f = GetFunc(GetRef(call));
 
   // Traverse subgraph inputs.
   auto input = VisitExpr(call->args[0]);
-  Array params;
-  Array args;
-  std::unordered_map params_bind;
-
-  // The subgraph may be merged so we need to update it again.
-  subgraph = GetSubgraph(GetRef(call));
-  CHECK(subgraph);
-
-  // Record the constants for propagation.
-  for (auto pair : subgraph->args) {
-params.push_back(pair.first);
-if (const auto* cn = pair.second.as()) {
-  params_bind[pair.first->name_hint()] = cn->data;
+  CHECK(region.defined()) << "Region not defined for " << 
GetRef(call);
+  // functions are created for each annotated regions,
+  // when their first output is encountered.
+  // If multiple outputs are there, a tuple node is inserted at the end.
+  // region_function_calls is map that maintains
+  // (each annotated regions) --> created function
+
+  if (region_function_calls.find(region) != region_function_calls.end()) {
+  // This section is executed only if there are multiple outputs in the 
region
+  // Thus, the function is always created and at the end there would be a 
tuple node
+  // Therefore, we insert a tuple get item node.
+
+  // Use the already created tuple node
+auto sg_call = region_function_calls[region];
+int index = GetRetIdx(region, GetRef(call));
+CHECK_NE(index, -1);
+
+auto tuple_get_item_ = TupleGetItem(sg_call, index);
+tuple_get_item_->checked_type_ = 
GetRef(call)->args[0]->checked_type_;
+return std::move(tuple_get_item_);
+  } else {
+// First time this region is encountered in the traversal
+// Creating the function
+
+Array fields;
+
+for (auto ret : region->GetOutputs()) {
+  auto ret_expr = VisitExpr(Downcast(ret)->args[0]);
+  fields.push_back(ret_expr);
+}
+int index = GetRetIdx(region, GetRef(call));
+CHECK_NE(index, -1);
+
+Array params;
+Array param_expr;
+std::unordered_map params_bind;
+
+for (auto pair : region_args[region]) {
+  params.push_back(pair.first);
+  if (const auto* cn = pair.second.as()) {
+params_bind[pair.first->name_hint()] = cn->data;
+  } else {
+ 

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5143: [RELAY] Re-wrote the Graph Partitioner to support multiple outputs

[GitBox]

zhiics commented on a change in pull request #5143: [RELAY] Re-wrote the Graph 
Partitioner to support multiple outputs
URL: https://github.com/apache/incubator-tvm/pull/5143#discussion_r397980929
 
 

 ##
 File path: src/relay/transforms/partition_graph.cc
 ##
 @@ -364,36 +388,114 @@ class Partitioner : public ExprMutator {
 
   IRModule Partition() {
 auto glob_funcs = module_->functions;
-for (const auto& pair : glob_funcs) {
-  if (auto* fn = pair.second.as()) {
+for (const auto  : glob_funcs) {
+  if (auto *fn = pair.second.as()) {
 auto func = GetRef(fn);
 func = Function(func->params,
-  VisitExpr(func->body),
-  func->ret_type,
-  func->type_params,
-  func->attrs);
+VisitExpr(func->body),
+func->ret_type,
+func->type_params,
+func->attrs);
 module_->Update(pair.first, func);
   }
 }
 return module_;
   }
 
  private:
-  int var_id_{0};
-  int subgraph_id_{0};
-  std::unordered_set> subgraphs_;
+  /*!
+  * \brief Get the region an expression belongs to
 
 Review comment:
   align "*", same to the other functions below.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5143: [RELAY] Re-wrote the Graph Partitioner to support multiple outputs

[GitBox]

zhiics commented on a change in pull request #5143: [RELAY] Re-wrote the Graph 
Partitioner to support multiple outputs
URL: https://github.com/apache/incubator-tvm/pull/5143#discussion_r397973058
 
 

 ##
 File path: src/relay/transforms/partition_graph.cc
 ##
 @@ -36,36 +36,23 @@
 #include 
 #include 
 
-#include 
+#include 
 #include 
 #include 
-#include 
 #include 
 
 #include "../backend/utils.h"
+#include "../analysis/annotated_region_set.h"
+
 
 namespace tvm {
 namespace relay {
 namespace partitioning {
 
 // Cache compiler_begin and compiler_end annotation ops for equivalence check 
to
 // reduce registry lookup overhead.
-static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
-static const Op& compiler_end_op = Op::Get("annotation.compiler_end");
-
-/*!
- * \brief The subgraph properties for partitioning.
- */
-struct Subgraph {
-  /*! \brief The subgraph ID. */
-  int id;
-
-  /*! \brief The input arguments of this subgraph. */
-  std::vector> args;
-
-  /*! \brief Nodes in this subgraph. */
-  std::unordered_set nodes;
-};
+static const Op _begin_op = Op::Get("annotation.compiler_begin");
 
 Review comment:
   keep the original code style: `static const Op&`
   
   Same for all the other places in the rest of PR


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5143: [RELAY] Re-wrote the Graph Partitioner to support multiple outputs

[GitBox]

zhiics commented on a change in pull request #5143: [RELAY] Re-wrote the Graph 
Partitioner to support multiple outputs
URL: https://github.com/apache/incubator-tvm/pull/5143#discussion_r397975848
 
 

 ##
 File path: src/relay/transforms/partition_graph.cc
 ##
 @@ -102,61 +89,71 @@ class AnnotationChecker : public ExprVisitor {
   bool found_end_{false};
 };
 
-/*! \brief This class partitions the expr labeled with begin and end annoations
+/*! \brief This class partitions the expr labeled with begin and end 
annotations
  * into function containing multiple regions. Each region is labeled with
  * a compiler attribute so that it will be handled by any compilers that are 
not
  * in the TVM stack.
  *
- * TODO(@zhiics) This following algorithm is not adequate to handle all cases,
- * i.e. multiple `compiler_end` nodes.
+ * Input : A Relay module that have functions with disjoint annotated regions
+ * using compiler_begin and compiler_end. There could be multiple 
outputs.
+ *
+ * Output : A Relay module with global functions for such disjoint annotated 
regions
+ *  with calls inserted at the respective location
+ *
+ * Dependencies : RegionSet Utility class.
+ *
+ * Methodology :
+ *  1) The RegionSet utility class is able to construct a collection of
+ * nodes that are bound by a given annotation -- here we use 
compiler_begin
+ * and compiler_end
+ *  2) Initially, for each function in the module RegionSets are populated.
+ *  3) Then, Vistor pass is traversed until a compiler_end node is 
encountered
+ * that belongs to a "region".
+ *  4) When the first compiler_end of a given annotated region is found, a 
function is
+ * formed and inserted.
+ * a) if the region has multiple outputs, a Tuple node (capturing all 
outputs)
+ *is returned.
+ *  5) Thereafter, if we encounter an another output of the same annotated 
region,
+ * it is important to note that the function is already formed. 
Therefore, it will
+ * lookup the function and add a TupleGetItemNode.
+ *  a) We will use the location index of "rets" of each "Region" of 
RegionSet
+ * as TupleGetItemNode index.
+ *  6) Therefore, functions will be created for all annotated regions. The 
name for each
+ * global function is created using "Region" id and the compiler name.
+ *
+ * Expected Usecase :
+ *  This pass is intended to run as the last pass in a series of passes as 
follows :
+ *  1) Annotate Supported Single Ops - annotated each single op with 
supported backends.
 
 Review comment:
   1) and 2) are inaccurate, we don't have these two annotations


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5143: [RELAY] Re-wrote the Graph Partitioner to support multiple outputs

[GitBox]

zhiics commented on a change in pull request #5143: [RELAY] Re-wrote the Graph 
Partitioner to support multiple outputs
URL: https://github.com/apache/incubator-tvm/pull/5143#discussion_r397982123
 
 

 ##
 File path: tests/python/relay/test_pass_partition_graph.py
 ##
 @@ -678,6 +678,81 @@ def expected():
 check_result(mod, {"y": y_data}, (8, 8), np.log(np_add))
 
 
+def test_multiple_outputs():
 
 Review comment:
   Can you add a test case for something like `conv2d + batch_norm`?


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on issue #5060: [uTVM][Runtime] Deprecate uTVM Standalone Runtime

[GitBox]

tqchen commented on issue #5060: [uTVM][Runtime] Deprecate uTVM Standalone 
Runtime
URL: https://github.com/apache/incubator-tvm/issues/5060#issuecomment-603910501
 
 
   @liangfu can you try to do a arena based approach given that it is simpler? 
We could adopt the counter based approach to enable early free of 
sub-arenas(when the free counters in the arena decreases to zero, we can free 
the space)


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] Hzfengsy commented on a change in pull request #5092: [TIR][PASS] dtype rewrite for indexing variables

[GitBox]

Hzfengsy commented on a change in pull request #5092: [TIR][PASS] dtype rewrite 
for indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r397949397
 
 

 ##
 File path: src/tir/pass/unroll_loop.cc
 ##
 @@ -160,7 +160,9 @@ class LoopUnroller : public StmtExprMutator {
 PrimExpr extent = tir::Simplify(op->extent);
 const IntImmNode  *v1 = extent.as();
 int value = -1;
-if (v1 != nullptr) {
+// integers that do not fit in int32_t are treated as symbolic,
+// as it's impossible to unroll such large loops
+if (v1 != nullptr && v1->value <= std::numeric_limits::max()) {
 
 Review comment:
   I wonder if we should use `int32_t` here rather than `int`. I'm not sure, 
but just worry about `int` will represent different types (`int16_t`, `int32_t` 
or `int64_t`) on different systems and devices.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on issue #5052: Relay to ONNX converter

[GitBox]

tqchen commented on issue #5052: Relay to ONNX converter
URL: https://github.com/apache/incubator-tvm/pull/5052#issuecomment-603908227
 
 
   I agree that target is possibly a better name


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen edited a comment on issue #5052: Relay to ONNX converter

[GitBox]

tqchen edited a comment on issue #5052: Relay to ONNX converter
URL: https://github.com/apache/incubator-tvm/pull/5052#issuecomment-603908227
 
 
   I agree that target is possibly a better name, we can discuss in the RFC


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] siju-samuel opened a new pull request #5147: [CRT]crt error handling

[GitBox]

siju-samuel opened a new pull request #5147: [CRT]crt error handling
URL: https://github.com/apache/incubator-tvm/pull/5147
 
 
   @liangfu @tmoreau89  Please help to review. TIA
   Most of the places error was not returned to caller and the function will 
continue to execute and cause some exception. This is handled.
   
   Thanks for contributing to TVM!   Please refer to guideline 
https://docs.tvm.ai/contribute/ for useful information and tips. After the pull 
request is submitted, please request code reviews from 
[Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers)
 by @ them in the pull request thread.
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen edited a comment on issue #5092: [TIR][PASS] dtype rewrite for indexing variables

[GitBox]

tqchen edited a comment on issue #5092: [TIR][PASS] dtype rewrite for indexing 
variables
URL: https://github.com/apache/incubator-tvm/pull/5092#issuecomment-603902373
 
 
   Some more comments, mainly wrt to clarity of the code, test coverage and 
efficiency concerns. Thanks for bringing in the PR. given that this is critical 
to a lot of the codebase, let us try to 
https://docs.tvm.ai/contribute/code_review.html#hold-the-highest-standard :) 
Let us work to polish it to the best state. Thanks @hzfan for good work so far


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on issue #5092: [TIR][PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on issue #5092: [TIR][PASS] dtype rewrite for indexing 
variables
URL: https://github.com/apache/incubator-tvm/pull/5092#issuecomment-603902755
 
 
   @spectrometerHBH @FrozenGene @Hzfengsy please also help to take a look


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on issue #5092: [PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on issue #5092: [PASS] dtype rewrite for indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#issuecomment-603902373
 
 
   Some more comments, mainly wrt to clarity of the code, test coverage and 
efficiency concerns. Thanks for bringing in the PR. given that this is critical 
to a lot of the codebase :) Let us work to polish it to the best state. Thanks 
@hzfan for good work so far


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for 
indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r397939075
 
 

 ##
 File path: src/tir/pass/narrow_datatype.cc
 ##
 @@ -0,0 +1,373 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file narrow_datatype.cc
+ * \brief narrow the datatype of indexing vars
+ */
+
+#include 
+#include 
+#include "../../arith/ir_mutator_with_analyzer.h"
+#include "../../arith/ir_visitor_with_analyzer.h"
+
+namespace tvm {
+namespace tir {
+
+// This pass narrows indexing expressions (like StoreNode::Index)
+// that trivially fit into i32 to i32. Considering that i32 indices
+// may be more efficient on some backends (while i64 may be more
+// efficient on others, like llvm), we may want this pass when i32
+// indices are more efficient.
+//
+// For Var v, we determine its dtype by examining all the PrimExpr
+// that contains v, denoted by E = {e_0 = v, e_1, e_2, ..., e_k}.
+// If all expressions in E fit into i32, then we think v can be narrowed
+// to i32.
+//
+// To make an indexing expression i32, we must make sure that every
+// component of that expression is of dtype i32. So besides Var, we
+// rewrite the following inside an indexing expression
+// - Var
+// - IntImm
+// - Cast
+//
+// Algorithm:
+// - Use DataTypeVisitor to determine whether a Var can be narrowed or not.
+// - Use DataTypeRewritter to rewrite the components of an indexing expression.
+
+using arith::Analyzer;
+using arith::IRMutatorWithAnalyzer;
+using arith::ConstIntBound;
+
+class DataTypeVisitor final : public StmtExprVisitor {
+ public:
+  explicit DataTypeVisitor(int target_bits)
+: bits_(target_bits), target_bits_(target_bits) {}
+
+  void VisitExpr(const PrimExpr& e) {
+if (e.dtype().is_int()) {
+  int bits = max_bits_;
+  ConstIntBound bound = analyzer_.const_int_bound(e);
 
 Review comment:
   NOTE: the constant int bound here is not necessarily the most efficient for 
deep nested expressions.
   
   As we are recursively calling const int bound for all sub-expressions of e 
as well(when we recursively visit). Perhaps we want to add a const int bound 
with memoization option that allows the analyzer to pass a memo(of each subexpr 
to the const int bound).
   
   We could add it as a TODO item, or directly do it in this PR, but would be 
great to be resolved in next few weeks  cc @yzhliu 
   
   
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for 
indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r397929962
 
 

 ##
 File path: tests/python/unittest/test_tir_pass_narrow_datatype.py
 ##
 @@ -0,0 +1,128 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import tvm
+from tvm import te
+
+
+def lower(sch, args):
+binds = {}
+arg_list = []
+for x in args:
+if isinstance(x, te.tensor.Tensor):
+buf = tvm.tir.decl_buffer(x.shape, dtype=x.dtype, name=x.name)
+assert x not in binds
+binds[x] = buf
+arg_list.append(buf)
+else:
+raise ValueError("args must be Tensor, Buffer or Var")
+bounds = te.schedule.InferBound(sch)
+stmt = te.schedule.ScheduleOps(sch, bounds)
+stmt = tvm.tir.ir_pass.StorageFlatten(stmt, binds, 64, False)
+stmt = tvm.tir.ir_pass.NarrowDataType(stmt, 32)
 
 Review comment:
   It would be great if we can add more test cases:
   
   - Please also consider use ir_builde to directly build loops
   - Have testcase that narrows to i16
   - Have a loop variable occurs in multiple expressions, one expr overflows, 
another does not
   
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for 
indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r397930633
 
 

 ##
 File path: tests/python/unittest/test_tir_pass_narrow_datatype.py
 ##
 @@ -0,0 +1,128 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import tvm
+from tvm import te
+
+
+def lower(sch, args):
+binds = {}
+arg_list = []
+for x in args:
+if isinstance(x, te.tensor.Tensor):
+buf = tvm.tir.decl_buffer(x.shape, dtype=x.dtype, name=x.name)
+assert x not in binds
+binds[x] = buf
+arg_list.append(buf)
+else:
+raise ValueError("args must be Tensor, Buffer or Var")
+bounds = te.schedule.InferBound(sch)
+stmt = te.schedule.ScheduleOps(sch, bounds)
+stmt = tvm.tir.ir_pass.StorageFlatten(stmt, binds, 64, False)
+stmt = tvm.tir.ir_pass.NarrowDataType(stmt, 32)
+return stmt
+
+
+def test_const():
+def test(m, n, dtype):
 
 Review comment:
   test -> check, because test can sometimes be picked up by test tools like 
nose


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for 
indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r397935092
 
 

 ##
 File path: src/tir/pass/narrow_datatype.cc
 ##
 @@ -0,0 +1,373 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file narrow_datatype.cc
+ * \brief narrow the datatype of indexing vars
+ */
+
+#include 
+#include 
+#include "../../arith/ir_mutator_with_analyzer.h"
+#include "../../arith/ir_visitor_with_analyzer.h"
+
+namespace tvm {
+namespace tir {
+
+// This pass narrows indexing expressions (like StoreNode::Index)
+// that trivially fit into i32 to i32. Considering that i32 indices
+// may be more efficient on some backends (while i64 may be more
+// efficient on others, like llvm), we may want this pass when i32
+// indices are more efficient.
+//
+// For Var v, we determine its dtype by examining all the PrimExpr
+// that contains v, denoted by E = {e_0 = v, e_1, e_2, ..., e_k}.
+// If all expressions in E fit into i32, then we think v can be narrowed
+// to i32.
+//
+// To make an indexing expression i32, we must make sure that every
+// component of that expression is of dtype i32. So besides Var, we
+// rewrite the following inside an indexing expression
+// - Var
+// - IntImm
+// - Cast
+//
+// Algorithm:
+// - Use DataTypeVisitor to determine whether a Var can be narrowed or not.
+// - Use DataTypeRewritter to rewrite the components of an indexing expression.
+
+using arith::Analyzer;
+using arith::IRMutatorWithAnalyzer;
+using arith::ConstIntBound;
+
+class DataTypeVisitor final : public StmtExprVisitor {
+ public:
+  explicit DataTypeVisitor(int target_bits)
+: bits_(target_bits), target_bits_(target_bits) {}
+
+  void VisitExpr(const PrimExpr& e) {
+if (e.dtype().is_int()) {
+  int bits = max_bits_;
+  ConstIntBound bound = analyzer_.const_int_bound(e);
+  int64_t ubound = Downcast(max_value(DataType::Int(target_bits_)))->value;
+  int64_t lbound = Downcast(min_value(DataType::Int(target_bits_)))->value;
+  if (e.dtype().bits() <= target_bits_ ||
 
 Review comment:
   Shall we rewrite lower bits into higher ones? cc @yzhliu 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for 
indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r397939075
 
 

 ##
 File path: src/tir/pass/narrow_datatype.cc
 ##
 @@ -0,0 +1,373 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file narrow_datatype.cc
+ * \brief narrow the datatype of indexing vars
+ */
+
+#include 
+#include 
+#include "../../arith/ir_mutator_with_analyzer.h"
+#include "../../arith/ir_visitor_with_analyzer.h"
+
+namespace tvm {
+namespace tir {
+
+// This pass narrows indexing expressions (like StoreNode::Index)
+// that trivially fit into i32 to i32. Considering that i32 indices
+// may be more efficient on some backends (while i64 may be more
+// efficient on others, like llvm), we may want this pass when i32
+// indices are more efficient.
+//
+// For Var v, we determine its dtype by examining all the PrimExpr
+// that contains v, denoted by E = {e_0 = v, e_1, e_2, ..., e_k}.
+// If all expressions in E fit into i32, then we think v can be narrowed
+// to i32.
+//
+// To make an indexing expression i32, we must make sure that every
+// component of that expression is of dtype i32. So besides Var, we
+// rewrite the following inside an indexing expression
+// - Var
+// - IntImm
+// - Cast
+//
+// Algorithm:
+// - Use DataTypeVisitor to determine whether a Var can be narrowed or not.
+// - Use DataTypeRewritter to rewrite the components of an indexing expression.
+
+using arith::Analyzer;
+using arith::IRMutatorWithAnalyzer;
+using arith::ConstIntBound;
+
+class DataTypeVisitor final : public StmtExprVisitor {
+ public:
+  explicit DataTypeVisitor(int target_bits)
+: bits_(target_bits), target_bits_(target_bits) {}
+
+  void VisitExpr(const PrimExpr& e) {
+if (e.dtype().is_int()) {
+  int bits = max_bits_;
+  ConstIntBound bound = analyzer_.const_int_bound(e);
 
 Review comment:
   NOTE: the constant int bound here is not necessarily the most efficient for 
deep nested expressions.
   
   As we are recursively calling const int bound for all sub-expressions of e 
as well(when we recursively visit). Perhaps we want to add a const int bound 
with memoization option that allows the analyzer to pass a memo(of each subexpr 
to the const int bound).
   
   This can be added as a TODO item, but would be great to be resolved in next 
few weeks  cc @yzhliu 
   
   
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for 
indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r397932487
 
 

 ##
 File path: src/tir/pass/narrow_datatype.cc
 ##
 @@ -0,0 +1,373 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file narrow_datatype.cc
+ * \brief narrow the datatype of indexing vars
+ */
+
+#include 
+#include 
+#include "../../arith/ir_mutator_with_analyzer.h"
+#include "../../arith/ir_visitor_with_analyzer.h"
+
+namespace tvm {
+namespace tir {
+
+// This pass narrows indexing expressions (like StoreNode::Index)
+// that trivially fit into i32 to i32. Considering that i32 indices
+// may be more efficient on some backends (while i64 may be more
+// efficient on others, like llvm), we may want this pass when i32
+// indices are more efficient.
+//
+// For Var v, we determine its dtype by examining all the PrimExpr
+// that contains v, denoted by E = {e_0 = v, e_1, e_2, ..., e_k}.
+// If all expressions in E fit into i32, then we think v can be narrowed
+// to i32.
+//
+// To make an indexing expression i32, we must make sure that every
+// component of that expression is of dtype i32. So besides Var, we
+// rewrite the following inside an indexing expression
+// - Var
+// - IntImm
+// - Cast
+//
+// Algorithm:
+// - Use DataTypeVisitor to determine whether a Var can be narrowed or not.
+// - Use DataTypeRewritter to rewrite the components of an indexing expression.
+
+using arith::Analyzer;
+using arith::IRMutatorWithAnalyzer;
+using arith::ConstIntBound;
+
+class DataTypeVisitor final : public StmtExprVisitor {
+ public:
+  explicit DataTypeVisitor(int target_bits)
+: bits_(target_bits), target_bits_(target_bits) {}
+
+  void VisitExpr(const PrimExpr& e) {
+if (e.dtype().is_int()) {
+  int bits = max_bits_;
+  ConstIntBound bound = analyzer_.const_int_bound(e);
+  int64_t ubound = Downcast(max_value(DataType::Int(target_bits_)))->value;
 
 Review comment:
   You only need to do Downcast(the second argument is deduced 
automatically)


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for 
indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r397937697
 
 

 ##
 File path: src/tir/pass/narrow_datatype.cc
 ##
 @@ -0,0 +1,373 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file narrow_datatype.cc
+ * \brief narrow the datatype of indexing vars
+ */
+
+#include 
+#include 
+#include "../../arith/ir_mutator_with_analyzer.h"
+#include "../../arith/ir_visitor_with_analyzer.h"
+
+namespace tvm {
+namespace tir {
+
+// This pass narrows indexing expressions (like StoreNode::Index)
+// that trivially fit into i32 to i32. Considering that i32 indices
+// may be more efficient on some backends (while i64 may be more
+// efficient on others, like llvm), we may want this pass when i32
+// indices are more efficient.
+//
+// For Var v, we determine its dtype by examining all the PrimExpr
+// that contains v, denoted by E = {e_0 = v, e_1, e_2, ..., e_k}.
+// If all expressions in E fit into i32, then we think v can be narrowed
+// to i32.
+//
+// To make an indexing expression i32, we must make sure that every
+// component of that expression is of dtype i32. So besides Var, we
+// rewrite the following inside an indexing expression
+// - Var
+// - IntImm
+// - Cast
+//
+// Algorithm:
+// - Use DataTypeVisitor to determine whether a Var can be narrowed or not.
+// - Use DataTypeRewritter to rewrite the components of an indexing expression.
+
+using arith::Analyzer;
+using arith::IRMutatorWithAnalyzer;
+using arith::ConstIntBound;
+
+class DataTypeVisitor final : public StmtExprVisitor {
 
 Review comment:
   Please document:
   
   - input, what would vmap eventually store
   - The general algorithm we use(e.g. we propagate the bits backwards into 
vmap)


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for 
indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r397936976
 
 

 ##
 File path: src/tir/pass/narrow_datatype.cc
 ##
 @@ -0,0 +1,373 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file narrow_datatype.cc
+ * \brief narrow the datatype of indexing vars
+ */
+
+#include 
+#include 
+#include "../../arith/ir_mutator_with_analyzer.h"
+#include "../../arith/ir_visitor_with_analyzer.h"
+
+namespace tvm {
+namespace tir {
+
+// This pass narrows indexing expressions (like StoreNode::Index)
+// that trivially fit into i32 to i32. Considering that i32 indices
+// may be more efficient on some backends (while i64 may be more
+// efficient on others, like llvm), we may want this pass when i32
+// indices are more efficient.
+//
+// For Var v, we determine its dtype by examining all the PrimExpr
+// that contains v, denoted by E = {e_0 = v, e_1, e_2, ..., e_k}.
+// If all expressions in E fit into i32, then we think v can be narrowed
+// to i32.
+//
+// To make an indexing expression i32, we must make sure that every
+// component of that expression is of dtype i32. So besides Var, we
+// rewrite the following inside an indexing expression
+// - Var
+// - IntImm
+// - Cast
+//
+// Algorithm:
+// - Use DataTypeVisitor to determine whether a Var can be narrowed or not.
+// - Use DataTypeRewritter to rewrite the components of an indexing expression.
+
+using arith::Analyzer;
+using arith::IRMutatorWithAnalyzer;
+using arith::ConstIntBound;
+
+class DataTypeVisitor final : public StmtExprVisitor {
+ public:
+  explicit DataTypeVisitor(int target_bits)
+: bits_(target_bits), target_bits_(target_bits) {}
+
+  void VisitExpr(const PrimExpr& e) {
+if (e.dtype().is_int()) {
+  int bits = max_bits_;
+  ConstIntBound bound = analyzer_.const_int_bound(e);
+  int64_t ubound = Downcast(max_value(DataType::Int(target_bits_)))->value;
+  int64_t lbound = Downcast(min_value(DataType::Int(target_bits_)))->value;
+  if (e.dtype().bits() <= target_bits_ ||
+  (bound->max_value <= ubound && bound->min_value >= lbound)) {
+bits = target_bits_;
+  }
+  int tmp = bits > bits_ ? bits :  bits_;
+  std::swap(bits_, tmp);
+  StmtExprVisitor::VisitExpr(e);
+  std::swap(bits_, tmp);
+} else {
+  StmtExprVisitor::VisitExpr(e);
+}
+  }
+
+  void VisitStmt_(const ForNode* op) {
+analyzer_.Bind(op->loop_var,
+   Range::make_by_min_extent(op->min, op->extent));
+vextent_[op->loop_var.as()] = op->extent.dtype();
+return StmtExprVisitor::VisitStmt_(op);
+  }
+
+  void VisitStmt_(const AttrStmtNode* op) {
+if (op->attr_key == attr::thread_extent ||
+op->attr_key == attr::virtual_thread) {
+  IterVar iv = Downcast(op->node);
+  CHECK_NE(iv->thread_tag.length(), 0U);
+  analyzer_.Bind(iv->var,
+  Range::make_by_min_extent(0, op->value));
+  vextent_[iv->var.as()] = op->value.dtype();
+  StmtExprVisitor::VisitStmt_(op);
+} else {
+  StmtExprVisitor::VisitStmt_(op);
+}
+  }
+
+  void VisitExpr_(const ReduceNode* op) {
+// Setup the domain information before simplification.
+for (const IterVar& iv : op->axis) {
+  analyzer_.Bind(iv->var, iv->dom);
+  vextent_[iv->var.as()] = iv->dom->extent.dtype();
+}
+// Recursively call simplification when necessary.
+StmtExprVisitor::VisitExpr_(op);
+  }
+
+  void VisitExpr_(const VarNode* op) {
+if (vextent_.find(op) != vextent_.end()) {
+  int bits = std::min(vextent_[op].bits(), bits_);
 
 Review comment:
   Add more comments about the algorithm


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for indexing variables

[GitBox]

tqchen commented on a change in pull request #5092: [PASS] dtype rewrite for 
indexing variables
URL: https://github.com/apache/incubator-tvm/pull/5092#discussion_r397936885
 
 

 ##
 File path: src/tir/pass/narrow_datatype.cc
 ##
 @@ -0,0 +1,373 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file narrow_datatype.cc
+ * \brief narrow the datatype of indexing vars
+ */
+
+#include 
+#include 
+#include "../../arith/ir_mutator_with_analyzer.h"
+#include "../../arith/ir_visitor_with_analyzer.h"
+
+namespace tvm {
+namespace tir {
+
+// This pass narrows indexing expressions (like StoreNode::Index)
+// that trivially fit into i32 to i32. Considering that i32 indices
+// may be more efficient on some backends (while i64 may be more
+// efficient on others, like llvm), we may want this pass when i32
+// indices are more efficient.
+//
+// For Var v, we determine its dtype by examining all the PrimExpr
+// that contains v, denoted by E = {e_0 = v, e_1, e_2, ..., e_k}.
+// If all expressions in E fit into i32, then we think v can be narrowed
+// to i32.
+//
+// To make an indexing expression i32, we must make sure that every
+// component of that expression is of dtype i32. So besides Var, we
+// rewrite the following inside an indexing expression
+// - Var
+// - IntImm
+// - Cast
+//
+// Algorithm:
+// - Use DataTypeVisitor to determine whether a Var can be narrowed or not.
+// - Use DataTypeRewritter to rewrite the components of an indexing expression.
+
+using arith::Analyzer;
+using arith::IRMutatorWithAnalyzer;
+using arith::ConstIntBound;
+
+class DataTypeVisitor final : public StmtExprVisitor {
+ public:
+  explicit DataTypeVisitor(int target_bits)
+: bits_(target_bits), target_bits_(target_bits) {}
+
+  void VisitExpr(const PrimExpr& e) {
+if (e.dtype().is_int()) {
+  int bits = max_bits_;
+  ConstIntBound bound = analyzer_.const_int_bound(e);
+  int64_t ubound = Downcast(max_value(DataType::Int(target_bits_)))->value;
+  int64_t lbound = Downcast(min_value(DataType::Int(target_bits_)))->value;
+  if (e.dtype().bits() <= target_bits_ ||
+  (bound->max_value <= ubound && bound->min_value >= lbound)) {
+bits = target_bits_;
+  }
+  int tmp = bits > bits_ ? bits :  bits_;
+  std::swap(bits_, tmp);
+  StmtExprVisitor::VisitExpr(e);
+  std::swap(bits_, tmp);
+} else {
+  StmtExprVisitor::VisitExpr(e);
+}
+  }
+
+  void VisitStmt_(const ForNode* op) {
+analyzer_.Bind(op->loop_var,
+   Range::make_by_min_extent(op->min, op->extent));
+vextent_[op->loop_var.as()] = op->extent.dtype();
+return StmtExprVisitor::VisitStmt_(op);
+  }
+
+  void VisitStmt_(const AttrStmtNode* op) {
+if (op->attr_key == attr::thread_extent ||
+op->attr_key == attr::virtual_thread) {
+  IterVar iv = Downcast(op->node);
+  CHECK_NE(iv->thread_tag.length(), 0U);
+  analyzer_.Bind(iv->var,
+  Range::make_by_min_extent(0, op->value));
+  vextent_[iv->var.as()] = op->value.dtype();
+  StmtExprVisitor::VisitStmt_(op);
+} else {
+  StmtExprVisitor::VisitStmt_(op);
+}
+  }
+
+  void VisitExpr_(const ReduceNode* op) {
+// Setup the domain information before simplification.
+for (const IterVar& iv : op->axis) {
+  analyzer_.Bind(iv->var, iv->dom);
+  vextent_[iv->var.as()] = iv->dom->extent.dtype();
+}
+// Recursively call simplification when necessary.
+StmtExprVisitor::VisitExpr_(op);
+  }
+
+  void VisitExpr_(const VarNode* op) {
+if (vextent_.find(op) != vextent_.end()) {
+  int bits = std::min(vextent_[op].bits(), bits_);
+  if (vmap.find(op) == vmap.end()) {
+vmap[op] = op->dtype.with_bits(bits);
+  } else {
+vmap[op] = op->dtype.with_bits(std::max(vmap[op].bits(), bits));
 
 Review comment:
   It would be great to add more comment here. e.g. We are taking maximum bits 
for all the possible Exprs that a Var occurs.


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 

[GitHub] [incubator-tvm] tqchen commented on issue #5145: [BUG][TUTORIAL] Tutorial for quantization need update

[GitBox]

tqchen commented on issue #5145:  [BUG][TUTORIAL] Tutorial for quantization 
need update
URL: https://github.com/apache/incubator-tvm/issues/5145#issuecomment-603892205
 
 
   feel free to send a PR to fix 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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] tqchen commented on issue #4459: [RUNTIME] Implement TVMDSOOp(TensorFlow custom op) for TVM runtime

[GitBox]

tqchen commented on issue #4459: [RUNTIME] Implement TVMDSOOp(TensorFlow custom 
op) for TVM runtime
URL: https://github.com/apache/incubator-tvm/pull/4459#issuecomment-603885462
 
 
   @yzhliu @zhiics @soiferj please follow up again. I will also take a look 
during the weekend


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] kumasento commented on issue #4847: Return empty CSourceModule when no lowered_funcs exists in Relay mod

[GitBox]

kumasento commented on issue #4847: Return empty CSourceModule when no 
lowered_funcs exists in Relay mod
URL: https://github.com/apache/incubator-tvm/pull/4847#issuecomment-603859603
 
 
   @trevor-m a tentative fix has been posted in #5146 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] kumasento opened a new pull request #5146: Handle empty LLVMModule in GetFunction

[GitBox]

kumasento opened a new pull request #5146: Handle empty LLVMModule in 
GetFunction
URL: https://github.com/apache/incubator-tvm/pull/5146
 
 
   #4847 introduces empty `LLVMModule`, which may appear when all the 
executable code are optimized #4748 . An empty `LLVMModule` is built from an IR 
file that only specifies target triplet and data layout.
   
   We just notice that existing logic in `LLVMModuleNode` may not consider this 
case properly. As shown by @trevor-m in this 
[comment](https://github.com/apache/incubator-tvm/pull/4847#issuecomment-600221459),
 `LLVMModuleNode::GetFunction` doesn't work properly.
   
   We dived into this problem and found out that `LLVMModuleNode` always 
assumes there exists an entry function (see this 
[line](https://github.com/apache/incubator-tvm/blob/3aabbd9c30d247a31eb19ebf997d6074b14b5dd9/src/target/llvm/llvm_module.cc#L321-L322)).
 If there is not, e.g., in an empty `LLVMModule`, we would have trouble, mostly 
in the form of segfault.
   
   This PR mainly deals with the empty `LLVMModule` issue in 
`LLVMModuleNode::GetFunction`. We assume that, without losing generality, a 
`LLVMModule` is empty if its `entry_func_` cannot be found, i.e.:
   
   ```c++
   GetGlobalAddr(runtime::symbol::tvm_module_main) == 0
   ```
   
   We use this assumption as follows:
   
   1. When `LazyInitJIT` is called by `GetFunction`, we will validate the 
address of `entry_func_` (by the condition above) first before we refer to it. 
   2. If `entry_func_` is empty, we can realize the current `LLVMModule` is 
empty and we should return `nullptr`.
   
   Please let me know whether this help address your problem @trevor-m 


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] mbaret commented on a change in pull request #5030: [RELAY] Added a AnnotatedRegion utility class

[GitBox]

mbaret commented on a change in pull request #5030: [RELAY] Added a 
AnnotatedRegion utility class
URL: https://github.com/apache/incubator-tvm/pull/5030#discussion_r397873579
 
 

 ##
 File path: python/tvm/relay/analysis/annotated_regions.py
 ##
 @@ -0,0 +1,62 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=no-else-return, unidiomatic-typecheck, invalid-name, 
unused-import
+"""Regions used in Relay."""
+
+from tvm.runtime import Object
+from . import _ffi_api
+
+
+class AnnotatedRegionSet(Object):
+"""Class to represent a relay expression split into regions."""
+
+def __init__(self, expr, region_begin_op, region_end_op):
+"""Construct regions from an expression.
+
+Parameters
+--
+expr : tvm.relay.Expr
+The expression from which to construct the regions.
+region_begin_op : tvm.relay.Op
+The region begin annotation.
+region_end_op : tvm.relay.Op
+The region end annotation.
+
+"""
+self.__init_handle_by_constructor__(_ffi_api.AnnotatedRegionSet,
+expr,
+region_begin_op,
+region_end_op)
+
+def __len__(self):
+return len(self.regions)
+
+def get_region(self, expr):
+"""Get the region an expression belongs to.
+
+Parameters
+--
+expr : tvm.relay.Expr
+The expression.
+
+Returns
+---
+region : Region
 
 Review comment:
   Removed 'Region'.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] mbaret commented on a change in pull request #5030: [RELAY] Added a AnnotatedRegion utility class

[GitBox]

mbaret commented on a change in pull request #5030: [RELAY] Added a 
AnnotatedRegion utility class
URL: https://github.com/apache/incubator-tvm/pull/5030#discussion_r397865495
 
 

 ##
 File path: src/relay/analysis/annotated_region_set.cc
 ##
 @@ -0,0 +1,239 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "annotated_region_set.h"
+
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+
+namespace tvm {
+namespace relay {
+
+AnnotatedRegion AnnotatedRegionSetNode::GetRegion(const Expr& expr) const {
+  for (auto candidate : regions_) {
+if (candidate->nodes.find(expr) != candidate->nodes.end()) {
+  return candidate;
+}
+  }
+  return AnnotatedRegion(nullptr);
+}
+
+void AnnotatedRegionSetNode::MergeRegions(AnnotatedRegion region1,
+  AnnotatedRegion region2) {
+  if (region1 == region2) {
+return;
+  }
+
+  // Merge region 2 to region 1 and erase region 2.
+  region1->nodes.insert(region2->nodes.begin(), region2->nodes.end());
+  for (auto arg : region2->ins) {
+region1->ins.push_back(arg);
+  }
+  for (auto out : region2->outs) {
+region1->outs.push_back(out);
+  }
+  // if any of the outputs of 2 are inputs of 1, they become internal nodes
+  // so remove them from outs/args
+  std::vector args_to_remove;
+  for (const auto& arg : region1->ins) {
+auto call = Downcast(arg);
+auto it = std::find(region2->outs.begin(), region2->outs.end(), 
call->args[0]);
+if (it != region2->outs.end()) {
+  args_to_remove.push_back(arg);
+  region1->outs.remove(*it);
+}
+  }
+  for (const auto& arg : args_to_remove) {
+region1->ins.remove(arg);
+  }
+  regions_.erase(region2);
+}
+
+void AnnotatedRegionSetNode::AddToRegion(AnnotatedRegion region, const Expr& 
expr) {
+  auto region2 = GetRegion(expr);
+  if (region2.defined()) {
+MergeRegions(region, region2);
+  } else {
+region->nodes.insert(expr);
+  }
+}
+
+AnnotatedRegion AnnotatedRegionSetNode::MakeRegion() {
 
 Review comment:
   I think now I've moved the region_id counting logic into it (as per 
comaniac's suggestion) it's existence becomes justified.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] kumasento commented on issue #4847: Return empty CSourceModule when no lowered_funcs exists in Relay mod

[GitBox]

kumasento commented on issue #4847: Return empty CSourceModule when no 
lowered_funcs exists in Relay mod
URL: https://github.com/apache/incubator-tvm/pull/4847#issuecomment-603793596
 
 
   Hi @trevor-m 
   Thanks for this information. I ran the test and reproduced the bug. I've 
located that the segfault should be raised from the `create` function of 
`llvm::EngineBuilder` (this 
[line](https://github.com/apache/incubator-tvm/blob/master/src/target/llvm/llvm_module.cc#L316)).
   
   Now I'm looking at the internal logic in LLVM to find out what the actual 
cause is. Please bear with me for 1-2 days.


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] liangfu commented on issue #5060: [uTVM][Runtime] Deprecate uTVM Standalone Runtime

[GitBox]

liangfu commented on issue #5060: [uTVM][Runtime] Deprecate uTVM Standalone 
Runtime
URL: https://github.com/apache/incubator-tvm/issues/5060#issuecomment-603746709
 
 
   
![](https://cdn-learn.adafruit.com/assets/assets/000/083/179/medium800/adafruit_products_image.png)
   
   It's very interesting to see tflite is using arena like allocator for 
micro-controllers. See how adafruit demonstrate its [PyBadge 
board](https://www.adafruit.com/product/4200) with TFLite 
[here](https://learn.adafruit.com/tensorflow-lite-for-edgebadge-kit-quickstart?view=all).


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #5030: [RELAY] Added a AnnotatedRegion utility class

[GitBox]

manupa-arm commented on a change in pull request #5030: [RELAY] Added a 
AnnotatedRegion utility class
URL: https://github.com/apache/incubator-tvm/pull/5030#discussion_r397711380
 
 

 ##
 File path: tests/python/relay/test_annotated_regions.py
 ##
 @@ -0,0 +1,121 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=no-else-return, unidiomatic-typecheck, invalid-name
+from tvm import relay
+from tvm.relay.op.annotation import compiler_begin, compiler_end
+
+
+def check_region(region_set, args, nodes, rets):
+region = region_set.get_region(args[0])
+assert region
+assert set(args) == set(region.args)
+assert set(nodes) == set(region.nodes)
+assert set(rets) == set(region.rets)
+
+
+def test_region_set_creator_diamond():
+data = relay.var('data', shape=(10, 10))
+cb_1 = compiler_begin(data, 'test_target')
+O_1 = relay.abs(cb_1)
+ce_1 = compiler_end(O_1, 'test_target')
+ce_2 = compiler_end(O_1, 'test_target')
+cb_2 = compiler_begin(ce_1, 'test_target')
+O_2 = relay.nn.relu(cb_2)
+ce_3 = compiler_end(O_2, 'test_target')
+cb_d = compiler_begin(ce_2, "default")
+X = relay.tanh(cb_d)
+ce_d = compiler_end(X, 'default')
+cb_3 = compiler_begin(ce_3, 'test_target')
+cb_4 = compiler_begin(ce_d, 'test_target')
+O_3 = relay.add(cb_3, cb_4)
+ce_4 = compiler_end(O_3, 'test_target')
+diamond = relay.Function([data], ce_4)
+
+region_set = relay.analysis.AnnotatedRegionSet(diamond,
+   
relay.op.get("annotation.compiler_begin"),
+   
relay.op.get("annotation.compiler_end"))
+assert len(region_set) == 4
+check_region(
+region_set,
+[cb_1],
+[cb_1, O_1, ce_1, ce_2],
+[ce_1, ce_2],
+)
+check_region(
+region_set,
+[cb_2],
+[cb_2, O_2, ce_3],
+[ce_3],
+)
+check_region(
+region_set,
+[cb_d],
+[cb_d, X, ce_d],
+[ce_d],
+)
+check_region(
+region_set,
+[cb_3, cb_4],
+[cb_3, cb_4, O_3, ce_4],
+[ce_4],
+)
+
+
+def test_region_set_creator_merged():
+data = relay.var('data', shape=(10, 10))
+cb_1 = compiler_begin(data, 'test_target')
+O_1 = relay.abs(cb_1)
+ce_2 = compiler_end(O_1, 'test_target')
+O_2 = relay.nn.relu(O_1)
+ce_3 = compiler_end(O_2, 'test_target')
+cb_d = compiler_begin(ce_2, "default")
+X = relay.tanh(cb_d)
+ce_d = compiler_end(X, 'default')
+cb_3 = compiler_begin(ce_3, 'test_target')
+cb_4 = compiler_begin(ce_d, 'test_target')
+O_3 = relay.add(cb_3, cb_4)
+ce_4 = compiler_end(O_3, 'test_target')
+merged = relay.Function([data], ce_4)
+
+region_set = relay.analysis.AnnotatedRegionSet(merged,
 
 Review comment:
   @jwfromm To clarify further,
   
   I think this feature provides a mechanism to obtain annotated regions (as a 
data structure --  a collection of nodes with ins and outs) that are bound 
(i.e. all the incoming and outgoing edges, to the region from the rest of the 
graph, are bound ) by annotation ops.
   
   One important use case of it is to highlight regions that are off-loaded to 
different backend compilers. In which case,  all the ins and outs (which are 
annotation ops) should have the 'compiler" attribute as @mbaret  mentions in 
the previous comment.
   
   However, it is not limited to backend compiler partitioning. I see this 
being useful in future region specific optimizations that a user might want to 
do without needing to pass additional objects (C++/Python); just through 
passing the IR modules with annotations. I am also using this in my recent 
[PR](https://github.com/apache/incubator-tvm/pull/5143).


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #5030: [RELAY] Added a AnnotatedRegion utility class

[GitBox]

manupa-arm commented on a change in pull request #5030: [RELAY] Added a 
AnnotatedRegion utility class
URL: https://github.com/apache/incubator-tvm/pull/5030#discussion_r397711380
 
 

 ##
 File path: tests/python/relay/test_annotated_regions.py
 ##
 @@ -0,0 +1,121 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=no-else-return, unidiomatic-typecheck, invalid-name
+from tvm import relay
+from tvm.relay.op.annotation import compiler_begin, compiler_end
+
+
+def check_region(region_set, args, nodes, rets):
+region = region_set.get_region(args[0])
+assert region
+assert set(args) == set(region.args)
+assert set(nodes) == set(region.nodes)
+assert set(rets) == set(region.rets)
+
+
+def test_region_set_creator_diamond():
+data = relay.var('data', shape=(10, 10))
+cb_1 = compiler_begin(data, 'test_target')
+O_1 = relay.abs(cb_1)
+ce_1 = compiler_end(O_1, 'test_target')
+ce_2 = compiler_end(O_1, 'test_target')
+cb_2 = compiler_begin(ce_1, 'test_target')
+O_2 = relay.nn.relu(cb_2)
+ce_3 = compiler_end(O_2, 'test_target')
+cb_d = compiler_begin(ce_2, "default")
+X = relay.tanh(cb_d)
+ce_d = compiler_end(X, 'default')
+cb_3 = compiler_begin(ce_3, 'test_target')
+cb_4 = compiler_begin(ce_d, 'test_target')
+O_3 = relay.add(cb_3, cb_4)
+ce_4 = compiler_end(O_3, 'test_target')
+diamond = relay.Function([data], ce_4)
+
+region_set = relay.analysis.AnnotatedRegionSet(diamond,
+   
relay.op.get("annotation.compiler_begin"),
+   
relay.op.get("annotation.compiler_end"))
+assert len(region_set) == 4
+check_region(
+region_set,
+[cb_1],
+[cb_1, O_1, ce_1, ce_2],
+[ce_1, ce_2],
+)
+check_region(
+region_set,
+[cb_2],
+[cb_2, O_2, ce_3],
+[ce_3],
+)
+check_region(
+region_set,
+[cb_d],
+[cb_d, X, ce_d],
+[ce_d],
+)
+check_region(
+region_set,
+[cb_3, cb_4],
+[cb_3, cb_4, O_3, ce_4],
+[ce_4],
+)
+
+
+def test_region_set_creator_merged():
+data = relay.var('data', shape=(10, 10))
+cb_1 = compiler_begin(data, 'test_target')
+O_1 = relay.abs(cb_1)
+ce_2 = compiler_end(O_1, 'test_target')
+O_2 = relay.nn.relu(O_1)
+ce_3 = compiler_end(O_2, 'test_target')
+cb_d = compiler_begin(ce_2, "default")
+X = relay.tanh(cb_d)
+ce_d = compiler_end(X, 'default')
+cb_3 = compiler_begin(ce_3, 'test_target')
+cb_4 = compiler_begin(ce_d, 'test_target')
+O_3 = relay.add(cb_3, cb_4)
+ce_4 = compiler_end(O_3, 'test_target')
+merged = relay.Function([data], ce_4)
+
+region_set = relay.analysis.AnnotatedRegionSet(merged,
 
 Review comment:
   @jwfromm To clarify further,
   
   I think feature provides a mechanism to obtain annotated regions (as a data 
structure --  a collection of nodes with ins and outs) that are bound (i.e. all 
the incoming and outgoing edges, to the region from the rest of the graph, are 
bound ) by annotation ops.
   
   One important use case of it is to highlight regions that are off-loaded to 
different backend compilers. In which case,  all the ins and outs (which are 
annotation ops) should have the 'compiler" attribute as @mbaret  mentions in 
the previous comment.
   
   However, it is not limited to backend compiler partitioning. I see this 
being useful in future region specific optimizations that a user might want to 
do without needing to pass additional objects (C++/Python); just through 
passing the IR modules with annotations. I am also using this in my recent 
[PR](https://github.com/apache/incubator-tvm/pull/5143).


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #5030: [RELAY] Added a AnnotatedRegion utility class

[GitBox]

manupa-arm commented on a change in pull request #5030: [RELAY] Added a 
AnnotatedRegion utility class
URL: https://github.com/apache/incubator-tvm/pull/5030#discussion_r397711380
 
 

 ##
 File path: tests/python/relay/test_annotated_regions.py
 ##
 @@ -0,0 +1,121 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=no-else-return, unidiomatic-typecheck, invalid-name
+from tvm import relay
+from tvm.relay.op.annotation import compiler_begin, compiler_end
+
+
+def check_region(region_set, args, nodes, rets):
+region = region_set.get_region(args[0])
+assert region
+assert set(args) == set(region.args)
+assert set(nodes) == set(region.nodes)
+assert set(rets) == set(region.rets)
+
+
+def test_region_set_creator_diamond():
+data = relay.var('data', shape=(10, 10))
+cb_1 = compiler_begin(data, 'test_target')
+O_1 = relay.abs(cb_1)
+ce_1 = compiler_end(O_1, 'test_target')
+ce_2 = compiler_end(O_1, 'test_target')
+cb_2 = compiler_begin(ce_1, 'test_target')
+O_2 = relay.nn.relu(cb_2)
+ce_3 = compiler_end(O_2, 'test_target')
+cb_d = compiler_begin(ce_2, "default")
+X = relay.tanh(cb_d)
+ce_d = compiler_end(X, 'default')
+cb_3 = compiler_begin(ce_3, 'test_target')
+cb_4 = compiler_begin(ce_d, 'test_target')
+O_3 = relay.add(cb_3, cb_4)
+ce_4 = compiler_end(O_3, 'test_target')
+diamond = relay.Function([data], ce_4)
+
+region_set = relay.analysis.AnnotatedRegionSet(diamond,
+   
relay.op.get("annotation.compiler_begin"),
+   
relay.op.get("annotation.compiler_end"))
+assert len(region_set) == 4
+check_region(
+region_set,
+[cb_1],
+[cb_1, O_1, ce_1, ce_2],
+[ce_1, ce_2],
+)
+check_region(
+region_set,
+[cb_2],
+[cb_2, O_2, ce_3],
+[ce_3],
+)
+check_region(
+region_set,
+[cb_d],
+[cb_d, X, ce_d],
+[ce_d],
+)
+check_region(
+region_set,
+[cb_3, cb_4],
+[cb_3, cb_4, O_3, ce_4],
+[ce_4],
+)
+
+
+def test_region_set_creator_merged():
+data = relay.var('data', shape=(10, 10))
+cb_1 = compiler_begin(data, 'test_target')
+O_1 = relay.abs(cb_1)
+ce_2 = compiler_end(O_1, 'test_target')
+O_2 = relay.nn.relu(O_1)
+ce_3 = compiler_end(O_2, 'test_target')
+cb_d = compiler_begin(ce_2, "default")
+X = relay.tanh(cb_d)
+ce_d = compiler_end(X, 'default')
+cb_3 = compiler_begin(ce_3, 'test_target')
+cb_4 = compiler_begin(ce_d, 'test_target')
+O_3 = relay.add(cb_3, cb_4)
+ce_4 = compiler_end(O_3, 'test_target')
+merged = relay.Function([data], ce_4)
+
+region_set = relay.analysis.AnnotatedRegionSet(merged,
 
 Review comment:
   @jwfromm To clarify further,
   
   I think pass provides a mechanism to obtain annotated region (as a data 
structure --  a collection of nodes with ins and outs) that are bound (i.e. all 
the incoming and outgoing edges, to the region from the rest of the graph, are 
bound ) by annotation ops.
   
   One important use case of it is to highlight regions that are off-loaded to 
different backend compilers. In which case,  all the ins and outs (which are 
annotation ops) should have the 'compiler" attribute as @mbaret  mentions in 
the previous comment.
   
   However, it is not limited to backend compiler partitioning. I see this 
being useful in future region specific optimizations that a user might want to 
do without needing to pass additional objects (C++/Python); just through 
passing the IR modules with annotations. I am also using this in my recent 
[PR](https://github.com/apache/incubator-tvm/pull/5143).


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #5030: [RELAY] Added a AnnotatedRegion utility class

[GitBox]

manupa-arm commented on a change in pull request #5030: [RELAY] Added a 
AnnotatedRegion utility class
URL: https://github.com/apache/incubator-tvm/pull/5030#discussion_r397711380
 
 

 ##
 File path: tests/python/relay/test_annotated_regions.py
 ##
 @@ -0,0 +1,121 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=no-else-return, unidiomatic-typecheck, invalid-name
+from tvm import relay
+from tvm.relay.op.annotation import compiler_begin, compiler_end
+
+
+def check_region(region_set, args, nodes, rets):
+region = region_set.get_region(args[0])
+assert region
+assert set(args) == set(region.args)
+assert set(nodes) == set(region.nodes)
+assert set(rets) == set(region.rets)
+
+
+def test_region_set_creator_diamond():
+data = relay.var('data', shape=(10, 10))
+cb_1 = compiler_begin(data, 'test_target')
+O_1 = relay.abs(cb_1)
+ce_1 = compiler_end(O_1, 'test_target')
+ce_2 = compiler_end(O_1, 'test_target')
+cb_2 = compiler_begin(ce_1, 'test_target')
+O_2 = relay.nn.relu(cb_2)
+ce_3 = compiler_end(O_2, 'test_target')
+cb_d = compiler_begin(ce_2, "default")
+X = relay.tanh(cb_d)
+ce_d = compiler_end(X, 'default')
+cb_3 = compiler_begin(ce_3, 'test_target')
+cb_4 = compiler_begin(ce_d, 'test_target')
+O_3 = relay.add(cb_3, cb_4)
+ce_4 = compiler_end(O_3, 'test_target')
+diamond = relay.Function([data], ce_4)
+
+region_set = relay.analysis.AnnotatedRegionSet(diamond,
+   
relay.op.get("annotation.compiler_begin"),
+   
relay.op.get("annotation.compiler_end"))
+assert len(region_set) == 4
+check_region(
+region_set,
+[cb_1],
+[cb_1, O_1, ce_1, ce_2],
+[ce_1, ce_2],
+)
+check_region(
+region_set,
+[cb_2],
+[cb_2, O_2, ce_3],
+[ce_3],
+)
+check_region(
+region_set,
+[cb_d],
+[cb_d, X, ce_d],
+[ce_d],
+)
+check_region(
+region_set,
+[cb_3, cb_4],
+[cb_3, cb_4, O_3, ce_4],
+[ce_4],
+)
+
+
+def test_region_set_creator_merged():
+data = relay.var('data', shape=(10, 10))
+cb_1 = compiler_begin(data, 'test_target')
+O_1 = relay.abs(cb_1)
+ce_2 = compiler_end(O_1, 'test_target')
+O_2 = relay.nn.relu(O_1)
+ce_3 = compiler_end(O_2, 'test_target')
+cb_d = compiler_begin(ce_2, "default")
+X = relay.tanh(cb_d)
+ce_d = compiler_end(X, 'default')
+cb_3 = compiler_begin(ce_3, 'test_target')
+cb_4 = compiler_begin(ce_d, 'test_target')
+O_3 = relay.add(cb_3, cb_4)
+ce_4 = compiler_end(O_3, 'test_target')
+merged = relay.Function([data], ce_4)
+
+region_set = relay.analysis.AnnotatedRegionSet(merged,
 
 Review comment:
   @jwfromm To clarify further,
   
   I think pass provides a mechanism to obtain annotated regions (as a data 
structure --  a collection of nodes with ins and outs) that are bound (i.e. all 
the incoming and outgoing edges, to the region from the rest of the graph, are 
bound ) by annotation ops.
   
   One important use case of it is to highlight regions that are off-loaded to 
different backend compilers. In which case,  all the ins and outs (which are 
annotation ops) should have the 'compiler" attribute as @mbaret  mentions in 
the previous comment.
   
   However, it is not limited to backend compiler partitioning. I see this 
being useful in future region specific optimizations that a user might want to 
do without needing to pass additional objects (C++/Python); just through 
passing the IR modules with annotations. I am also using this in my recent 
[PR](https://github.com/apache/incubator-tvm/pull/5143).


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-tvm] mbaret commented on issue #5134: [RELAY] Add MergeCompilerRegions pass

[GitBox]

mbaret commented on issue #5134: [RELAY] Add MergeCompilerRegions pass
URL: https://github.com/apache/incubator-tvm/pull/5134#issuecomment-603702585
 
 
   It isn't (although I hope to handle that next). This is to help support the 
case where we partition into a region that has multiple outputs.


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:
us...@infra.apache.org


With regards,
Apache Git Services