[GitHub] [tvm] chunit-quic commented on a diff in pull request #13212: [TVMC][microNPU] tvmc option for printing which operators are offloaded to Ethos-U

["chunit-quic (via GitHub)" <gi...@apache.org>]

chunit-quic commented on code in PR #13212:
URL: https://github.com/apache/tvm/pull/13212#discussion_r1090188874


##########
python/tvm/relay/frontend/common.py:
##########
@@ -1067,6 +1067,20 @@ def __init__(self, span):
             self._span = tvm.relay.Span(tvm.relay.SourceName(span.decode("utf-8")), 0, 0, 0, 0)
         else:
             assert False, f"unsupported span type: {type(span)}"
+        self.suffix_str = "_PART_"
+        self.counter = 0
+        self.distance_from_leaf = -1
+
+    def _create_span(self):
+        """Adds suffix_str + counter value to _span.source_name.name,
+        to create a unique source_name for the Relay layer
+        """
+        if self.distance_from_leaf == 0:
+            return tvm.relay.Span(tvm.relay.SourceName(self._span), 0, 0, 0, 0)
+        self.distance_from_leaf -= 1
+        span_str = "{}{}{}".format(self._span.source_name.name, self.suffix_str, str(self.counter))
+        self.counter += 1
+        return tvm.relay.Span(tvm.relay.SourceName(span_str), 0, 0, 0, 0)

Review Comment:
   > This seems sensible to me, cc the original author of span filling @chunit-quic just to check
   
   Hi all,
   
   Thank you for informing us this change, we just check the PR. We have some ideas about it, and we would like to share some discussions about the suffix in the forum with you.
   
   About the way how span is used in the PR, it looks to me like you would like to take the
   source_name of span as an indicator to the original Relay call(expression). 
   We find the hash value might be enough for your use case rather than making a unique source_name for a span. Please allow me to describe some problems might be cuased by adding suffix.
   
   We removed suffix in the new implementation in our [preRFC](https://discuss.tvm.apache.org/t/pre-rfc-tvm-explorer-infrastructure/13457#reference-level-explanation-9). In short, we found adding suffix might mislead in some cases. Here is an example:
   
   Suppose:
   1. An user thinks span meaning where an expression is from
   2. A conv2d layer is converted to 2 relay exprs (conv2d and relu) in a specific frontend.
   
   The conversion of a simple frontend model with only a conv2d layer will result in this:
   
   ```python
   #FRONTEND
   conv2d layer, named as my_conv2d
   #Relay IR
   %0 = conv2d() /* my_conv2d_part_0 */
   %1 = relu() /* my_conv2d_part_1 */
   ```
   In the case above, that user might assume there are "two layers" in the original frontend model, rather than thinking the converted Relay exprs are from the same forntend layer.
   One is conv2d layer with the name "my_conv2d_part_0", and a relu layer which is named as
   "my_conv2d_part_1".
   
   Yet with help of suffix we do have some advantages. Like allocating the expression which generates the tensor output result of its frontend layer. So far we have [many discussions](https://discuss.tvm.apache.org/t/pre-rfc-tvm-explorer-infrastructure/13457/26?u=chunit)
   with community but we are still thinking about whether there is a better way to deal with.
   
   Therefore currently we won't recommend you to add suffix to the source_name of span. After checking the codes, perhaps you could consider using the hash value, `tvm.ir.structural_hash()` of an expr as the key of dictionary, like:
   
   ```python
   #python/tvm/relay/analysis/operations_distribution.py
   class AnalyzeOperationsDistribution(ExprVisitor):
       #...
       def visit_call(self, call: relay.Call):
           if isinstance(call.op, tvm.ir.Op):
               self.unique_op_ids[tvm.ir.structural_hash(call)] = [self.compiler_name, self.func_name, self.func_id]
   #python/tvm/driver/tvmc/compiler.py
   def dump_operation_offloads(...):
       def annotate_f(x):
           ret = ""
           if isinstance(x, relay.Call):
               compiler_name, op_name, func_id = operations_distribution[tvm.ir.structural_hash(x)]
               ret = f", func_id: {func_id}, compiler_name: {compiler_name}, op_name: {op_name}"
           return ret
   ```
   
   Because no rewriting/mutation is invoked in `dump_operation_offloads()`, hash value won't be changed. Maybe hash value is an acceptable alternative?
   
   Thank you for reading! Feel free if you have any question. We will try to get back to you soon. :)
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org