delldu commented on a change in pull request #8443:
URL: https://github.com/apache/tvm/pull/8443#discussion_r670339452



##########
File path: include/tvm/relay/attrs/nn.h
##########
@@ -1463,6 +1463,21 @@ struct NLLLossAttrs : public 
tvm::AttrsNode<NLLLossAttrs> {
   }
 };  // struct NLLLossAttrs
 
+/*! \brief Attributes used in Im2col operator */
+struct Im2colAttrs : public tvm::AttrsNode<Im2colAttrs> {
+  Array<IndexExpr> kernel_size;
+  Array<IndexExpr> dilation;
+  Array<IndexExpr> padding;
+  Array<IndexExpr> stride;

Review comment:
       Sorry let give a little explain.
   1)  Im2Compute must following TVM RELAY_REGISTER_OP interface, so it is
   ```
   Array<te::Tensor> Im2colCompute(const Attrs& attrs, const Array<te::Tensor>& 
inputs,
                                   const Type& out_type) {
     const auto* param = attrs.as<Im2colAttrs>();
     ICHECK(param != nullptr);
   
     return Array<te::Tensor>{
         topi::im2col(inputs[0], param->kernel_size, param->dilation, 
param->padding, param->stride)};
   }
   ```
   2. We have two choice, 1)parsing parameters in Im2colCompute, 2) parsing in 
topi::im2col, here we choice 2), that is in topi::im2col, there are no 
difference. 3) Change Im2colAttrs. 
   3. Struct Conv2DAttrs is also using IndexExpr instead of Integer, so 
**Im2colAttrs follow their style**, in fact, kernel_size mean kernel_size_h/w, 
dilation means dilation_h/w etc, not pure Integer.
   
   So RELAY_REGISTER_OP ==> Im2colCompute ==> im2col and Im2colAttrs, we almost 
have no choice.
   




-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to