[GitHub] [incubator-tvm] comaniac commented on a change in pull request #6724: [BYOC] [ACL] 20.05 memory corruption temporarely fix

[GitBox <gi...@apache.org>]

comaniac commented on a change in pull request #6724:
URL: https://github.com/apache/incubator-tvm/pull/6724#discussion_r509636638



##########
File path: python/tvm/relay/op/contrib/arm_compute_lib.py
##########
@@ -248,6 +251,7 @@ def conv2d(attrs, args):
     return True
 
 
+# conv2d does not need padding check in 20.05

Review comment:
       I cannot link this comment with the code.

##########
File path: python/tvm/relay/op/contrib/arm_compute_lib.py
##########
@@ -289,24 +293,57 @@ def qnn_dense(attrs, args):
         return False
     if attrs.out_dtype != "int32":
         return False
-    return True
+
+    return not padding_required([*args, out_type])
 
 
 @tvm.ir.register_op_attr("nn.max_pool2d", "target.arm_compute_lib")
-def max_pool2d(attrs, args):
+def max_pool2d(attrs, args, out_type):
     """Check if the external ACL codegen for maxpool2d should be used."""
     if attrs.layout != "NHWC":
         return False
     typ = args[0].checked_type
     if typ.dtype not in ["float32", "uint8"]:
         return False
-    return True
+    return not padding_required([*args, out_type])
+
+
+def padding_required(inputs):

Review comment:
       - `require_padding` might be more straightforward?
   - I feel that this checker can be more general to support both versions. For example, you can check the ACL version and always return True if it's < 20.05. In this way, this PR won't be a "temporary" fix anymore.
   - If we simply require ACL 20.11 or higher, can we get rid of this problem without any change?

##########
File path: include/tvm/relay/op_attr_types.h
##########
@@ -185,6 +185,20 @@ using FTVMLegalize = runtime::TypedPackedFunc<Expr(const Attrs& attrs, const Arr
 using FTVMAnnotateTarget = runtime::TypedPackedFunc<bool(const Attrs& attrs,  // NOLINT(*)
                                                          const Array<Expr>& args)>;
 
+/*!
+ * \brief Annotates an expression to indicate if an op should be compiled using
+ *  the given compiler/target.
+ *  \param attrs The attribute of the original expr.
+ *  \param args The arguments of the original expr.
+ *  \param out_type The return type of the original expr.
+ *
+ *  \return true if this op should be registered to invoke a specific compiler
+ *  for codegen, otherwise, false.
+ */
+using FTVMAnnotateTarget3 =

Review comment:
       It is not a good practice to have multiple annotator interfaces. If the type original expression is necessary, we may change the interface to `sing FTVMAnnotateTarget = runtime::TypedPackedFunc<bool(const Expr&)>;` that accepts the original expression directly. As a result, `attrs`, `args`, and `checked_type` can all be retrieved.

##########
File path: tests/python/contrib/test_arm_compute_lib/test_network.py
##########
@@ -151,7 +152,34 @@ def get_model():
     )
 
 
+# Test disabled as it requires attontate target not promote unannotated tuples under previous operations annotation

Review comment:
       ```suggestion
   # Test disabled as it requires annotate target not promote unannotated tuples under previous operations' annotation
   ```
   If this test cannot be executed in this PR, we should add it along with the one it can be.

##########
File path: python/tvm/relay/op/contrib/arm_compute_lib.py
##########
@@ -227,9 +229,10 @@ def _func_wrapper(attrs, args):
     return _func_wrapper
 
 
+# Reshape does not need padding check in 20.05

Review comment:
       I cannot link this comment with the code.

##########
File path: python/tvm/relay/op/contrib/arm_compute_lib.py
##########
@@ -289,24 +293,57 @@ def qnn_dense(attrs, args):
         return False
     if attrs.out_dtype != "int32":
         return False
-    return True
+
+    return not padding_required([*args, out_type])
 
 
 @tvm.ir.register_op_attr("nn.max_pool2d", "target.arm_compute_lib")
-def max_pool2d(attrs, args):
+def max_pool2d(attrs, args, out_type):
     """Check if the external ACL codegen for maxpool2d should be used."""
     if attrs.layout != "NHWC":
         return False
     typ = args[0].checked_type
     if typ.dtype not in ["float32", "uint8"]:
         return False
-    return True
+    return not padding_required([*args, out_type])
+
+
+def padding_required(inputs):
+    """Checks whether supplied data will require padding.
+    Most of the operators ACL up to 20.11 uses padded data.
+    """
+
+    def _check(shape, dtype):
+        """NEON has 128bits/16bytes per vector"""
+        if len(shape) == 0:
+            return False
+        return (shape[-1] * np.dtype(dtype).itemsize) % 16 != 0
+
+    def _padding_required():
+        for i in inputs:
+            if isinstance(i, (tvm.relay.expr.Var, tvm.relay.expr.Call)):
+                if _check(i.checked_type.shape, i.checked_type.dtype):
+                    return True
+            elif isinstance(i, tvm.relay.expr.Constant):
+                if _check(i.data.shape, i.data.dtype):
+                    return True
+            elif isinstance(i, tvm.ir.tensor_type.TensorType):
+                if _check(i.shape, i.dtype):
+                    return True
+            else:
+                raise Exception("Not supported")
+
+        return False
+
+    result = _padding_required()

Review comment:
       I didn't see the reason of using an internal function. You should be able to directly more L323-L336 out?

##########
File path: include/tvm/relay/op_attr_types.h
##########
@@ -176,7 +176,7 @@ using FTVMLegalize = runtime::TypedPackedFunc<Expr(const Attrs& attrs, const Arr
  * \brief Annotates an expression to indicate if an op should be compiled using
  * the given compiler/target.
  *
- * \param attrs The attribute of the original expr.
+ :* \param attrs The attribute of the original expr.

Review comment:
       Why adding a colon?
   

##########
File path: python/tvm/relay/op/contrib/arm_compute_lib.py
##########
@@ -227,9 +229,10 @@ def _func_wrapper(attrs, args):
     return _func_wrapper
 
 
+# Reshape does not need padding check in 20.05
 _register_external_op_helper("reshape")
 
-
+# conv2d does not need padding check in 20.05

Review comment:
       I cannot link this comment with 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:
users@infra.apache.org